Unexpected Behavior When Scanning Non-Existing and Latest Versions of npm Packages

[AmalChandru]

Description:

I was playing around with Vet locally and encountered a couple of issues when scanning one of my npm libraries:

1. The latest version of the package is 1.0.2. However, when scanning with that version, no vulnerabilities or issues were detected, despite known security vulnerabilities. Ideally, when the latest version of a package is provided, it should run and display results in the CLI. Is this expected behavior? [fig:1]
2. When I tried scanning with version 11.0.0 (which doesn’t exist), the system suggested updating to 1.0.2, even though 11.0.0 is not available. Ideally, Vet should flag an invalid version as feedback. Shouldn’t it? [fig:2]

Steps to reproduce:

1. Run vet scan --purl pkg:npm/[email protected]
2. Observe that no vulnerabilities are detected for a package with known security issues.
3. Run vet scan --purl pkg:npm/[email protected]
4. Observe the incorrect suggestion for an update to 1.0.2.

Expected behavior:

1. When scanning with the latest version, results should be displayed in the CLI.
2. When scanning with a non-existent version, the system should raise feedback indicating the version is invalid.

Screenshots:

[abhisek]

@AmalChandru Thanks for reporting this. Lets talk about [1] first.

I tried running

vet scan --purl pkg:npm/[email protected] --transitive --filter 'true'

This tells me the package and its transitive dependencies (upto a certain depth). I don't see any vulnerabilities reported by vet. Can you share how you identified "known vulnerabilities" in this version of the package?

When I run the same command but with our default policy, I do see some violations:

vet scan --purl pkg:npm/[email protected] --transitive --filter-suite ./samples/filter-suites/fs-generic.yml 

At this point, I am not sure how to check the known vulnerabilities for pkg:npm/[email protected] because I don't see anything on deps.dev or osv.dev. Can you help?

[AmalChandru]

I believe my choice of the term "known vulnerabilities" might have caused some confusion. What I intended to highlight is the major version drift in the direct dependency (can pose a security vulnerability), which is already being detected by Vet. As shown in Fig. 2, it's indicating that one library is out of date due to a major version drift in the direct dependencies.

[abhisek]

@AmalChandru I believe the bug related to major version drift especially where it is asking to update 11.0.0 (non-existent) version to 1.0.2 (latest known) is somewhere here:

https://github.com/safedep/vet/blob/main/pkg/reporter/summary.go#L447

This needs some analysis on how to handle this case.

[AmalChandru]

Thanks for the pointer @abhisek 🙏

Currently, the packageUpdateVersionForRemediationAdvicefunction uses semver.Diff to check for differences between the user-specified version and the current version. However, this approach can produce misleading outputs because it only checks for differences and does not validate whether the user-specified version exists or is valid. If the versions differ, it always returns the latest version (as shown in the printed console statements).

To improve this, what about adding a check to determine if the user-specified version is greater than the current version ? If it is, we can notify the user that the provided version is invalid or implement an alternative feedback mechanism.

Here’s an example of how this could be implemented:

// Check if the user input version is greater than the current version
userVersion, _ := semver.NewVersion(pkg.PackageDetails.Version)
currentVersion, _ := semver.NewVersion(insightsCurrentVersion)

if userVersion.GreaterThan(currentVersion) {
    fmt.Printf("Provided version %s is greater than the current version %s.\n", pkg.PackageDetails.Version, insightsCurrentVersion)
    return "Provided version is not valid."
}

[abhisek]

@AmalChandru How about change UPDATE TO column name to LATEST? This way we are using a more correct naming because "Latest Known" is really what is coming from the backend. This way the table message will be more clear and specific. What do you think?

Implementing GreaterThan check will work when version naming follow semver convention. There are a lot of packages that don't. Especially with Go where pseudo-version using timestamp & commit hash. https://go.dev/ref/mod#versions

Given the complexity in dealing with versions I think we should first fix the naming before dealing with version complexities. Feel free to raise a PR to fix the column name if you agree.

[AmalChandru]

@abhisek Addressing the column naming first makes sense before tackling the broader version-handling logic. I'll go ahead and raise a PR to update the column name.