feat: Implement Yarn Berry Analyser #7319

segovia · 2025-01-15T17:45:31Z

I have implemented an Analyzer to address the issue that DependencyCheck does not work with projects that use yarn berry (#4894). This is a first draft and I would be happy to implement tests and make any other modifications if there is interest to merge this to main.

marcelstoer · 2025-01-17T11:15:29Z

Thank you so much for this! This really simplifies our lives with multi-language projects driven by a Maven build (Java backend, JavaScript frontend). Btw, it might also fix #4215.

I think this can and should be improved to reduce future maintenance costs. Think about what happens when Yarn releases the next breaking change (like classic --> Berry)?

I would like to see a Yarn analyzer abstraction with some sort of auto-detection mechanism. IMO users should only have to set the yarnAuditAnalyzerEnabled without having to worry about the implementation. yarnAuditAnalyzerEnabled and yarnBerryAuditAnalyzerEnabled are actually mutually exclusive.

Can you extract a YarnAuditAnalyzer superclass with two subclasses for classic/v1 and Berry? Or, if they don't share enough, extract a YarnAuditAnalyzer interface with two implementations? They would need to be accompanied with a matching factory that introspects the lock file (or something else?) to instantiate the matching implementation.

/CC @chadlwilson @jeremylong

jeremylong · 2025-01-19T13:33:27Z

@marcelstoer completely agree - your suggestions are spot on.

segovia · 2025-01-24T16:04:42Z

With the updated PR, I have refactored YarnAuditAnalyzer into: AbstractYarnAuditAnalyzer, YarnClassicAuditAnalyzer and YarnBerryAuditAnalyzer, where the first contains shared analyzer code, the second is basically the same as the original YarnAuditAnalyzer and the third contains the Yarn Berry implementation.
As suggested, both analyzers share settings and are enabled or disabled with yarnAuditAnalyzerEnabled. They check the major version of yarn to determine if the Yarn Classic Analyzer should be used or the Berry, so they will run mutually exclusively.

jeremylong

LGTM - I left some suggestions around the yarn version number. I prefer to use something like semver4j as we already have it on the classpath.

jeremylong · 2025-01-25T12:41:30Z

core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractYarnAuditAnalyzer.java

+    protected String getYarnMajorVersion() {
+        if (StringUtils.isBlank(yarnVersion)) {
+            throw new IllegalArgumentException("Version string cannot be null or empty");
+        }
+        String[] parts = yarnVersion.split("\\.");
+        if (parts.length == 0) {
+            throw new IllegalArgumentException("Invalid version string format");
+        }
+        return parts[0];
+    }


Consider:

Suggested change

protected String getYarnMajorVersion() {

if (StringUtils.isBlank(yarnVersion)) {

throw new IllegalArgumentException("Version string cannot be null or empty");

}

String[] parts = yarnVersion.split("\\.");

if (parts.length == 0) {

throw new IllegalArgumentException("Invalid version string format");

}

return parts[0];

}

protected int getYarnMajorVersion() {

if (StringUtils.isBlank(yarnVersion)) {

throw new IllegalArgumentException("Version string cannot be null or empty");

}

try {

Semver semver = new Semver(yarnVersion);

return semver.getMajor();

} catch (SemverException e) {

throw new IllegalArgumentException("Invalid version string format", e);

}

}

jeremylong · 2025-01-25T12:41:58Z

core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractYarnAuditAnalyzer.java

 import org.owasp.dependencycheck.dependency.Dependency;
 import org.owasp.dependencycheck.exception.InitializationException;
 import org.owasp.dependencycheck.utils.FileFilterBuilder;
 import org.owasp.dependencycheck.utils.Settings;
-import org.owasp.dependencycheck.utils.URLConnectionFailureException;
 import org.owasp.dependencycheck.utils.processing.ProcessReader;
 import org.slf4j.Logger;


Suggested change

import org.slf4j.Logger;

import org.semver4j.Semver;

import org.semver4j.SemverException;

import org.slf4j.Logger;

jeremylong · 2025-01-25T12:42:58Z

core/src/main/java/org/owasp/dependencycheck/analyzer/YarnBerryAuditAnalyzer.java

+            return;
+        }
+        var yarnMajorVersion = getYarnMajorVersion();
+        if ("1".equals(yarnMajorVersion)) {


Suggested change

if ("1".equals(yarnMajorVersion)) {

if (yarnMajorVersion == 1) {

Changing the comparison isn't addressing the real issue IMO.

I'm with all those who see magic numbers or magic strings as an anti-pattern. The suggested fix usually is to use (properly) named constants. In this case, however, that would only improve the legibility of the code but would not improve its long-term stability.

At this point, all we know is that the Yarn classic analyzer supports Yarn v1 and that the Yarn Berry analyzer supports Yarn v2. We are not in a position to make any promises as for a potential future Yarn v3. The Berry analyzer may or may not support it. To play it safe, I'd like to see an ok-list approach.

Suggestion:

Declare an abstract supportedNodeMajorVersions() method in AbstractYarnAuditAnalyzer that returns an array of ints or a set of Ints.

Let both sub classes implement it to the best of their knowledge. Hence, both would currently return a set containing a single value.

Add a boolean isYarnVersionSupported() method to AbstractYarnAuditAnalyzer that compares supportedNodeVersions() against the return value of getYarnMajorVersion() (major contained in list of supported). Nice side effect: getYarnMajorVersion() can likely be made private.

The prepareFileTypeAnalyzer() implementations would thus be shielded from all that complexity as they would only need to do an if (!isYarnVersionSupported())-then-disable-analyzer.

This idea could be spun even further but that would require more refactoring.

Yarn v3 and v4 already exist, and are considered part of the 'berry' family. Current version is 4.6.0. It's reasonable to consider this analyzer will be required indefinitely.

Is this a theoretical objection, or are you familiar with modern Yarn and its compat/history?

We don't lock other analyzers against software versions that might be released in future. Given the yarn analyzer has been broken for years now with respect to current yarn I don't think adding more maintenance overhead to specifically check new versions is necessary?

As you might have guessed, I'm not at all familiar with anything related to Yarn. Also, I don't consider my comment to be an objection but merely a suggestion. When I scanned the PR initially, the magic numbers stuck out immediately.

It's reasonable to consider this analyzer will be required indefinitely.

If that's true, then my suggestion doesn't make much sense. Thanks for pointing that out.

Yup, no worries - appreciate the thoughtful input - even if theoretical :-) Magic numbers can indeed be a bit gross and it's reasonable to debate about Yarn's compatibility guarantees for this stuff over time.

It does occur to me to contemplate whether the "berry" analyzer as it is here works the same back through yarn 2 / 3 /4 as I cannot recall off the top of my head when it was introduced or whether format has changed.

Converting the comparison to use int will make it easier to implement a new analyzer if a successor to yarn berry has a different format.

jeremylong · 2025-01-25T12:43:50Z

core/src/main/java/org/owasp/dependencycheck/analyzer/YarnClassicAuditAnalyzer.java

+            return;
+        }
+        var yarnMajorVersion = getYarnMajorVersion();
+        if (!"1".equals(yarnMajorVersion)) {


Suggested change

if (!"1".equals(yarnMajorVersion)) {

if (yarnMajorVersion > 1) {

jeremylong · 2025-01-25T12:44:30Z

core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractYarnAuditAnalyzer.java

+    /**
+     * Extracts the major version from a version string.
+     *
+     * @return the major version (e.g., "4" from "4.2.1")


Suggested change

* @return the major version (e.g., "4" from "4.2.1")

* @return the major version (e.g., `4` from "4.2.1")

jeremylong

It looks like the build failed:

 YarnBerryAuditAnalyzerIT.testAnalyzePackageYarn:46 More than 1 dependency should be identified

Do we need to update the workflow itself to include yarn berry?

chadlwilson · 2025-01-25T12:58:36Z

Do we need to update the workflow itself to include yarn berry?

FWIW the canonical way to run and install yarn 2+ is via corepack and source controlling yarn itself alongside a package.json which defines the version number.

https://yarnpkg.com/getting-started/install

Although you can use yarn 1 to switch dynamically to yarn 2+.projects if you have to.

Ideally the tests should test a project that has package.json etc preconfigured for yarn usage and 'corepack enable' run on the npm install. This has the advantage that specific yarn versions are source controlled alongside the tests in the .yarn folder (canonically).

marcelstoer · 2025-01-25T20:29:42Z

@segovia thanks for the updated version. This is much more maintainable now!

boring-cyborg bot added ant changes to ant cli changes to the cli core changes to core documentation site documentation maven changes to the maven plugin utils changes to utils labels Jan 15, 2025

segovia mentioned this pull request Jan 15, 2025

Support yarn berry in Analyzer #4894

Open

feat: Implement Yarn Berry Analyser

9bced45

segovia force-pushed the yarn-berry branch from 7551e42 to 9bced45 Compare January 24, 2025 15:58

boring-cyborg bot added the tests test cases label Jan 24, 2025

jeremylong reviewed Jan 25, 2025

View reviewed changes

jeremylong requested changes Jan 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement Yarn Berry Analyser #7319

feat: Implement Yarn Berry Analyser #7319

segovia commented Jan 15, 2025 •

edited

Loading

marcelstoer commented Jan 17, 2025 •

edited

Loading

jeremylong commented Jan 19, 2025

segovia commented Jan 24, 2025

jeremylong left a comment

jeremylong Jan 25, 2025

jeremylong Jan 25, 2025

jeremylong Jan 25, 2025

marcelstoer Jan 25, 2025

chadlwilson Jan 26, 2025

marcelstoer Jan 26, 2025

chadlwilson Jan 26, 2025

jeremylong Jan 26, 2025

jeremylong Jan 25, 2025

marcelstoer Jan 25, 2025

jeremylong Jan 25, 2025

jeremylong left a comment

chadlwilson commented Jan 25, 2025

marcelstoer commented Jan 25, 2025

	if ("1".equals(yarnMajorVersion)) {
	if (yarnMajorVersion == 1) {

	if (!"1".equals(yarnMajorVersion)) {
	if (yarnMajorVersion > 1) {

	* @return the major version (e.g., "4" from "4.2.1")
	* @return the major version (e.g., `4` from "4.2.1")

feat: Implement Yarn Berry Analyser #7319

Are you sure you want to change the base?

feat: Implement Yarn Berry Analyser #7319

Conversation

segovia commented Jan 15, 2025 • edited Loading

marcelstoer commented Jan 17, 2025 • edited Loading

jeremylong commented Jan 19, 2025

segovia commented Jan 24, 2025

jeremylong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremylong left a comment

Choose a reason for hiding this comment

chadlwilson commented Jan 25, 2025

marcelstoer commented Jan 25, 2025

segovia commented Jan 15, 2025 •

edited

Loading

marcelstoer commented Jan 17, 2025 •

edited

Loading