-
Notifications
You must be signed in to change notification settings - Fork 11
feat(providers): add SHA-256 hash computation for Maven dependencies #612
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
31e1fc9
0690f65
1a75922
ecbf2c1
c8a54aa
09347e3
dbb8618
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import crypto from 'node:crypto' | ||
| import fs from 'node:fs' | ||
| import os from 'node:os' | ||
| import path from 'node:path' | ||
|
|
@@ -145,21 +146,100 @@ export default class Java_maven extends Base_java { | |
| if (process.env["TRUSTIFY_DA_DEBUG"] === "true") { | ||
| console.error("Dependency tree that will be used as input for creating the BOM =>" + EOL + EOL + content.toString()) | ||
| } | ||
| let sbom = this.createSbomFileFromTextFormat(content.toString(), ignoredDeps, opts, manifest); | ||
| const depTreeContent = content.toString() | ||
| const hashMap = this._buildMavenHashMap(depTreeContent, opts) | ||
| let sbom = this.createSbomFileFromTextFormat(depTreeContent, ignoredDeps, opts, manifest, hashMap); | ||
| // delete temp file and directory | ||
| fs.rmSync(tmpDir, { recursive: true, force: true }) | ||
| // return dependency graph as string | ||
| return sbom | ||
| } | ||
|
|
||
| /** @type {Object<string, string>} Packaging types that produce .jar files despite non-jar packaging names. */ | ||
| static PACKAGING_TO_JAR = { 'bundle': 'jar', 'eclipse-plugin': 'jar' } | ||
|
|
||
| /** | ||
| * Build a Map of PURL string → CycloneDX hash entries by reading artifact files from the local Maven repository. | ||
| * | ||
| * Both the artifact file path and the hash-map key are derived from the | ||
| * shared {@link Base_java#parseCoordinate} parser: the file path uses the | ||
| * resolved version and raw classifier, while the key uses the same canonical | ||
| * PURL builder as {@link Base_java#parseDep}. This guarantees the key always | ||
| * matches the PURL that {@link Base_java#parseDependencyTree} looks up. | ||
| * | ||
| * @param {string} depTreeText Raw dependency tree text from mvn dependency:tree | ||
| * @param {{}} [opts={}] Options bag (may contain TRUSTIFY_DA_MVN_REPO) | ||
| * @returns {Map<string, Array<{alg: string, content: string}>>} | ||
| */ | ||
| _buildMavenHashMap(depTreeText, opts = {}) { | ||
| const m2Repo = getCustom('TRUSTIFY_DA_MVN_REPO', path.join(os.homedir(), '.m2', 'repository'), opts) | ||
| const hashMap = new Map() | ||
| const lines = depTreeText.split(EOL) | ||
| // Track hash coverage so an incomplete .m2 cache (e.g. ephemeral CI | ||
| // containers or resolve-only phases) surfaces a summary warning instead | ||
| // of silently producing an SBOM with missing hashes. | ||
| let attempted = 0 | ||
| let missed = 0 | ||
|
|
||
| for (const rawLine of lines) { | ||
| const trimmed = rawLine.trim() | ||
| // Strip leading tree-drawing characters (e.g. "\-", "+-", "|") so the | ||
| // parenthesized-line check sees the coordinate itself. Omitted | ||
| // duplicate/conflict lines start with these characters after trim(), | ||
| // then an opening "(" — without stripping, the "(" check never fires. | ||
| const cleaned = trimmed.replace(/^[|+\\\- ]+/, '') | ||
| if (!cleaned || cleaned.startsWith('(')) { continue } | ||
|
|
||
| const coord = this.parseCoordinate(rawLine) | ||
| if (!coord.groupId || !coord.artifactId || !coord.packaging) { continue } | ||
| if (coord.packaging === 'pom') { continue } | ||
|
|
||
| // Key by the exact PURL parseDep() produces, so the lookup in | ||
| // parseDependencyTree (hashMap.get(to.toString())) always hits. | ||
| const purl = this._coordinateToPurl(coord).toString() | ||
| // The same artifact recurs across dependency-tree branches — notably in | ||
| // multi-module reactor builds, where every module re-lists shared deps. | ||
| // The digest is deterministic per PURL, so skip the redundant file read | ||
| // and SHA-256 computation once this PURL is already hashed. | ||
| if (hashMap.has(purl)) { continue } | ||
|
|
||
| const ext = Java_maven.PACKAGING_TO_JAR[coord.packaging] || coord.packaging | ||
| const groupPath = coord.groupId.replaceAll('.', path.sep) | ||
| const fileName = coord.classifier | ||
| ? `${coord.artifactId}-${coord.version}-${coord.classifier}.${ext}` | ||
| : `${coord.artifactId}-${coord.version}.${ext}` | ||
| const artifactPath = path.join(m2Repo, groupPath, coord.artifactId, coord.version, fileName) | ||
|
|
||
| attempted++ | ||
| try { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Efficiency: No deduplication guard before file I/O The same artifact can appear in multiple branches of the dependency tree. Combined with the dead 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 const purl = this.toPurl(groupId, artifactId, purlVersion).toString()
if (hashMap.has(purl)) { continue }
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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 fileContent = fs.readFileSync(artifactPath) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Efficiency: Large artifacts (e.g., 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [sdlc-workflow/verify-pr] Classified as suggestion — proposes streaming instead of |
||
| const digest = crypto.createHash('sha256').update(fileContent).digest('hex') | ||
| hashMap.set(purl, [{ alg: 'SHA-256', content: digest }]) | ||
| } catch { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Observability: Silent degradation with incomplete The Consider logging a summary warning at the end (e.g.,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| missed++ | ||
| if (process.env['TRUSTIFY_DA_DEBUG'] === 'true') { | ||
| console.error(`Maven hash: artifact not found at ${artifactPath}, omitting hash`) | ||
| } | ||
| } | ||
| } | ||
| // Mirror the pip provider's convention (python_controller.js): surface an | ||
| // unconditional warning when hashes could not be computed, so incomplete | ||
| // hash coverage is visible even without TRUSTIFY_DA_DEBUG. | ||
| if (missed > 0) { | ||
| console.warn(`Maven hash: ${missed} of ${attempted} artifacts could not be read from the local .m2 cache; SBOM will be generated without hashes for those components.`) | ||
| } | ||
| return hashMap | ||
| } | ||
|
|
||
| /** | ||
| * @param {String} textGraphList Text graph String of the manifest | ||
| * @param {[String]} ignoredDeps List of ignored dependencies to be omitted from sbom | ||
| * @param {{}} opts Options | ||
| * @param {String} manifestPath Path to the pom.xml manifest | ||
| * @param {Map<string, Array<{alg: string, content: string}>>} [hashMap] PURL→hashes map | ||
| * @return {String} formatted sbom Json String with all dependencies | ||
| */ | ||
| createSbomFileFromTextFormat(textGraphList, ignoredDeps, opts, manifestPath) { | ||
| createSbomFileFromTextFormat(textGraphList, ignoredDeps, opts, manifestPath, hashMap) { | ||
| let lines = textGraphList.split(EOL); | ||
| // get root component | ||
| let root = lines[0]; | ||
|
|
@@ -168,6 +248,9 @@ export default class Java_maven extends Base_java { | |
| let sbom = new Sbom(); | ||
| sbom.addRoot(rootPurl, license); | ||
| this.parseDependencyTree(root, 0, lines.slice(1), sbom); | ||
| // Attach Maven artifact hashes as a post-processing step, keeping this | ||
| // Maven-specific concern out of the shared dependency-tree parser. | ||
| sbom.attachHashes(hashMap); | ||
| return sbom.filterIgnoredDeps(ignoredDeps).getAsJsonString(opts); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Design: Duplicated coordinate parsing will silently drift
This method re-implements the split-by-colon,
DEP_REGEXapplication, classifier detection, and conflict-override handling thatparseDepinbase_java.js:98-117already does. The two implementations already diverge (scope lists, conflict-override classifier handling).Any future change to
parseDepthat 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 bothparseDepand_buildMavenHashMapcall, returning{groupId, artifactId, version, classifier, packaging, scope}. This eliminates the entire class of drift bugs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[sdlc-workflow/verify-pr] Classified as suggestion — proposes extracting a shared
parseCoordinatehelper. 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.