feat: [CMPA-603] Add npm support for SBOM component metadata being FF#6950
feat: [CMPA-603] Add npm support for SBOM component metadata being FF#6950calhar-snyk wants to merge 1 commit into
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
This comment has been minimized.
This comment has been minimized.
59984f8 to
2fe53c6
Compare
This comment has been minimized.
This comment has been minimized.
2fe53c6 to
5f7299b
Compare
This comment has been minimized.
This comment has been minimized.
5f7299b to
83d9058
Compare
This comment has been minimized.
This comment has been minimized.
83d9058 to
aff526d
Compare
This comment has been minimized.
This comment has been minimized.
aff526d to
35c9cf1
Compare
This comment has been minimized.
This comment has been minimized.
35c9cf1 to
714ee6c
Compare
This comment has been minimized.
This comment has been minimized.
714ee6c to
98c902f
Compare
This comment has been minimized.
This comment has been minimized.
98c902f to
fb0c5cb
Compare
This comment has been minimized.
This comment has been minimized.
| scanAllUnmanaged?: boolean; | ||
| showNpmScope?: boolean; | ||
| allProjects?: boolean; | ||
| includeComponentMetadata?: boolean; |
There was a problem hiding this comment.
can this hold values such as null and undefined? Just a sanity question
There was a problem hiding this comment.
Effectively yes, in the vast majority of cases it won't be getting set.
There was a problem hiding this comment.
https://github.com/snyk/cli/pull/6948/changes#diff-eb8513f7e557af1081e96db6b6603ca292cc0c2bf383fb00c5f914431372cee3 is where we build the Options object that gets passed in to the plugin inspect method. So can be undefined
| it('does not attach the labels without the flag', async () => { | ||
| const project = await createProjectFromFixture(fixture); | ||
|
|
||
| const { code, stdout } = await runSnykCLI( | ||
| 'test --print-graph --file=package-lock.json', | ||
| { cwd: project.path() }, | ||
| ); |
There was a problem hiding this comment.
since this test is in a loop, I'm wondering if we should add some timeout here or if default is enough
There was a problem hiding this comment.
I think the default timeout should be sufficient. The underlying behaviour is purely dependent on package-lock.json contents which are simple and small. This should be fast.
| expect(code).toEqual(0); | ||
| expect(stdout).toContain('DepGraph data:'); | ||
| expect(labelKeys(stdout, 'hash:')).toHaveLength(0); | ||
| expect(labelKeys(stdout, 'distribution:url')).toHaveLength(0); |
There was a problem hiding this comment.
is there any chance that changes in the responses from runSnykCLI would cause these values to fail the test?
Maybe a better question is: are we testing what we want here?
There was a problem hiding this comment.
(maybe my comment is hard to understand the way I wrote - just wondering if this could be flaky)
There was a problem hiding this comment.
I don't think this should ever be flakey. The results here should not be dependent on external services as the underlying flows rely entirely on the package-lock.json contents
fb0c5cb to
94820da
Compare
This comment has been minimized.
This comment has been minimized.
94820da to
953a6dd
Compare
This comment has been minimized.
This comment has been minimized.
953a6dd to
cc7531c
Compare
PR Reviewer Guide 🔍
|
|
|
||
| expect(code).toEqual(0); | ||
| const graphs = parseDepGraphs(stdout); | ||
| expect(graphs.map((g) => g.target).sort()).toEqual([root, ...members]); |
There was a problem hiding this comment.
This is actually an interesting problem as we should internally use /, so there might be some more central problem than just the assertion.
|
|
||
| expect(code).toEqual(0); | ||
| const graphs = parseDepGraphs(stdout); | ||
| expect(graphs.map((g) => g.target).sort()).toEqual([root, ...members]); |

Pull Request Submission Checklist
What does this PR do?
Extends
--include-component-metadatasupport to npm projects, following on from the Maven support added in #6948.When the flag is set, the npm plugin now forwards it to
snyk-nodejs-lockfile-parser, which reads the install-timeintegrityandresolvedfields already recorded in the lockfile and surfaces them ashash:<algorithm>anddistribution:urllabels on the dep-graph nodes. The SBOM extension then uses these labels to populate component hashes and external distribution URLs in the CycloneDX output.Concretely:
includeComponentMetadatathrough the legacynpm-lock-parserin both code paths — the v2/v3 dep-graph path (parseNpmLockV2Project) and the v1 depTree path (buildDepTreeFromFiles). For v1, the labels survive the downstream depTree→dep-graph conversion.includeComponentMetadatato the pluginOptionstype.snyk-nodejs-lockfile-parserto2.9.0, the version that emits the component-metadata labels.The flag remains internal/undocumented and is gated by the feature-flag gateway — the same surface as the Maven precursor. Unlike Maven, no artifact resolution step is needed: the metadata already lives in the lockfile, so nothing has to be installed/resolved first.
Where should the reviewer start?
src/lib/plugins/nodejs-plugin/npm-lock-parser.ts— the two-line forwarding change (v1 and v2/v3 paths).src/lib/plugins/types.ts— the new optionalOptionsfield.test/jest/acceptance/snyk-test/npm-include-component-metadata.spec.ts— coverage across lockfile v1/v2/v3.Note: npm intentionally stays on the in-tree legacy plugin here (the change just threads the flag through it).
How should this be manually tested?
Against an npm project with a
package-lock.json(v1, v2, or v3):Dep-graph nodes should carry
hash:<algorithm>anddistribution:urllabels. Re-running without--include-component-metadatashould produce no such labels.Automated coverage:
test/jest/acceptance/snyk-test/npm-include-component-metadata.spec.tsexercises lockfile v1/v2/v3, each asserting the labels are present with the flag and absent without it.The full
snyk sbomflow can also be tested against the pre-prod API:What's the product update that needs to be communicated to CLI users?
None. The feature is inert by default and gated behind an internal feature flag; there is no user-facing change unless explicitly enabled.
Risk assessment (Low | Medium | High)?
Low. The behaviour is gated behind an internal feature flag from the feature-flag gateway and is inert by default — clients see no change unless it is explicitly enabled for them. The only always-on change is the
snyk-nodejs-lockfile-parser2.9.0 bump, which adds the label-emitting code path without altering default output. npm scans continue to run through the existing legacy plugin.Any background context you want to provide?
Precursor: #6948 (Maven component metadata + registering
include-component-metadataas a recognised CLI argument forwarded to the plugins). This PR is the npm counterpart.What are the relevant tickets?