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 7 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.33% +0.10%
==========================================
Files 42 43 +1
Lines 9175 9645 +470
Branches 1624 1736 +112
==========================================
+ Hits 8370 8809 +439
- Misses 805 836 +31
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.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — sub-task TC-5646 created to address this feedback (grouped with the scope-list divergence; same PURL key-mismatch root cause).
|
|
||
| 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 }There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — sub-task TC-5647 created to address this feedback.
| 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.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as code change request — sub-task TC-5646 created to address this feedback (grouped with the conflict-override mismatch; same PURL key-mismatch root cause).
| * @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.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as suggestion — proposes extracting a shared parseCoordinate helper. This is the recommended fix approach and has been captured in the Implementation Notes of sub-task TC-5646, which addresses the concrete drift bugs. No separate sub-task created.
| : `${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 }There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as suggestion — a deduplication/performance optimization not required for correctness and not backed by a documented CONVENTIONS.md convention or an established codebase pattern. No sub-task created.
| 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.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as suggestion — proposes streaming instead of readFileSync; a performance optimization with no matching project convention or codebase pattern. No sub-task created.
| 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.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as suggestion — proposes logging a hash-coverage summary (observability); no logging/observability convention is documented and the current silent omission is intended graceful degradation. No sub-task created.
| * @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.
There was a problem hiding this comment.
[sdlc-workflow/verify-pr] Classified as suggestion — a design refinement (avoid a Maven-specific hashMap param in the shared base class); no convention or codebase pattern mandates it and the optional parameter is backward-compatible. No sub-task created.
|
[sdlc-workflow/verify-pr] Re: @ruromero review — Classified as code change request — the review summary consolidates findings that were classified and actioned individually. Must-fix bugs are tracked in sub-tasks TC-5646 (PURL key divergence: conflict-override + scope-list) and TC-5647 (dead parenthesized-line guard). The design, efficiency, and observability items were classified as suggestions (see per-line replies); no additional sub-task created for the summary itself. |
Verification Report for TC-5549 (commit 0690f65)PR: #612 — SHA-256 hash computation for Maven dependencies Guardrail & Domain Results
❗ Note on Verification Commands (FAIL)The FAIL is environmental / specification, not a defect in the PR code:
Because Verification Commands is a scored (non-informational) row, Overall aggregates to FAIL; reviewers should read this as "commands-as-written could not be executed" rather than "the implementation is broken." Code Change Requests → Sub-tasks
Root-Cause Tasks
This comment was AI-generated by sdlc-workflow/verify-pr v0.13.8. |
Extract a shared parseCoordinate() helper plus _coordinateToPurl() in Base_Java, and route both parseDep() and _buildMavenHashMap() through them. Previously the hash-map key builder duplicated the coordinate/PURL logic with a divergent scope list and re-appended the classifier after a conflict override, so classified Maven dependencies (and any whose version lost a conflict) produced a hash-map key that did not match the PURL parseDep() emits — silently dropping their SHA-256 hashes from the SBOM. The hash-map key is now derived from the same canonical builder as parseDep(), guaranteeing they cannot diverge. MAVEN_SCOPES moves to Base_Java as the single source of truth for scope detection. Implements TC-5646 Assisted-by: Claude Code
The parenthesized-line guard in _buildMavenHashMap checked
trimmed.startsWith('('), but after trim() the omitted duplicate/conflict
lines begin with tree-drawing characters (\-, +-, |), so the check never
fired — the guard was dead code and the test passed only because the
referenced jar was absent from the mock .m2 fixture.
Strip the leading tree-drawing characters before the check so omitted
entries are skipped by the guard itself, before any file I/O. Update the
duplicate/conflict tests to place the referenced jar in the mock .m2 so
they prove the guard (not a missing file) is what skips the entry.
Implements TC-5647
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Assisted-by: Claude Code
The same artifact recurs across dependency-tree branches — notably in multi-module reactor builds where every module re-lists shared deps. _buildMavenHashMap read and SHA-256-hashed the jar once per occurrence, producing the identical digest every time. Hoist the PURL computation above the file I/O and skip the redundant read + hash when the PURL is already in the map (the digest is deterministic per PURL). Addresses review feedback on PR guacsec#612 (efficiency: no deduplication guard before file I/O). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Assisted-by: Claude Code
The catch block in _buildMavenHashMap swallowed every read failure unless TRUSTIFY_DA_DEBUG was set. In CI environments with partial .m2 caches (ephemeral containers, resolve-only phases), every readFileSync can fail silently, producing an SBOM with zero hashes and no visible signal. Track attempted vs. missed reads and emit a single summary warning at the end when any artifact could not be read, so hash coverage is visible even in non-debug mode. Mirrors the pip provider's unconditional console.warn convention (python_controller.js). The dedup guard already runs before the counter, so recurring reactor deps do not inflate the totals. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Assisted-by: Claude Code
The hashMap parameter threaded through Base_java.parseDependencyTree was a Maven-only concern leaking into the shared abstraction: Gradle extends Base_java, uses its own tree parser, and never passes a hash map. Remove hashMap from parseDependencyTree and attach Maven artifact hashes as a post-processing step instead. A new generic Sbom.attachHashes(hashMap) walks the built components and sets hashes by matching PURL, without overwriting hashes already present. The generic component-hash capability on addDependency (CycloneDX 1.4) is unchanged; only the base parser is cleaned. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Assisted-by: Claude Code
Apply two lessons from the Maven SHA PR (guacsec#612) to the Gradle provider: Key drift (lesson guacsec#1): derive both the stored hash-map key and the lookup key from the canonical PURL (`toPurl(...).toString()` / `purl.toString()`), the same builder parseDep uses, so the two keys cannot drift in formatting. Refactor hashKeyFromComponentId into parseComponentId (parse/validate only); the class method builds the canonical key. Degradation warning (lesson guacsec#2): parseGradleHashes previously degraded completely silently on every failure path. Emit a console.warn when gradle cannot be invoked, the init script fails, hashing fails, or some resolved artifacts cannot be read (with an attempted/missed count summary), mirroring the pip/cargo providers so incomplete hash coverage is visible without TRUSTIFY_DA_DEBUG. Add regression tests (groovy + kotlin): canonical-key round trip for a conflict-resolved (`->`) transitive dependency, and warning emission on the partial-miss and failing-init-script paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3 participants
Summary
.m2/repository/cacheTRUSTIFY_DA_MVN_REPOenv var_buildMavenHashMapto prevent malformed.m2paths (review feedback fix)parseCoordinate()/_coordinateToPurl()helper inBase_Java, and route bothparseDep()and_buildMavenHashMap()through it — the hash-map key can no longer drift from the PURLparseDep()emits, fixing silent hash loss for classified dependencies and conflict-overridden versions (review feedback fix)\-,+-,|) before the parenthesized-line guard in_buildMavenHashMapso omitted duplicate/conflict lines are skipped by the guard itself — before any file I/O — instead of relying on a missing fixture jar; duplicate/conflict tests now place the referenced jar in the mock.m2to prove the guard is what skips the entry (review feedback fix)_buildMavenHashMap: hoist the PURL computation and skip the redundantreadFileSync+ SHA-256 when a PURL is already hashed — the same artifact recurs across dependency-tree branches (notably multi-module reactor builds), and the digest is deterministic per PURL (review feedback fix)_buildMavenHashMap: track attempted vs. missed artifact reads and emit a single summary warning (N of M artifacts could not be read…) even withoutTRUSTIFY_DA_DEBUG, so partial.m2caches in CI (ephemeral containers, resolve-only phases) no longer produce a hashless SBOM silently — mirrors the pip provider'sconsole.warnconvention (review feedback fix)hashMapparameter fromBase_java.parseDependencyTree(Gradle extends the base but uses its own parser and never passed it) and attach hashes as a Maven post-processing step via a new genericSbom.attachHashes(hashMap)that matches components by PURL without overwriting existing hashes (review feedback fix)Implements TC-5549
Implements TC-5581
Implements TC-5646
Implements TC-5647
Test plan
🤖 Generated with Claude Code