Skip to content
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.

Sign up for GitHub

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

scan and report dependency groups of vulnerabilities #655

Merged
merged 20 commits into from
Dec 11, 2023
Merged

scan and report dependency groups of vulnerabilities #655

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
5e3e396
scan and report dependency groups of vulnerabilities
cuixq Sep 28, 2023
04ab40b
lint
cuixq Nov 15, 2023
7f1b8d9
Merge branch 'main' into group
cuixq Nov 15, 2023
75b86f3
Merge branch 'google:main' into group
cuixq Nov 16, 2023
cc9b3ad
make dependency groups a slice of string
cuixq Nov 15, 2023
81584d5
lint
cuixq Nov 16, 2023
b1e5ec0
no need to deep reflect PackageInfo
cuixq Nov 21, 2023
8e3dc4f
Merge branch 'main' into group
cuixq Nov 22, 2023
4912b30
make license allowlist matching case insensitive (#672)
josieang Nov 23, 2023
3e0ba51
refactor: upgrade golangci-lint (#673)
G-Rath Nov 23, 2023
0b0fa42
fix(deps): update osv-scanner minor (#652)
renovate-bot Nov 23, 2023
c1214c4
make dependency groups a slice of string
cuixq Nov 15, 2023
5bf495e
add method IsDevGroup for Ecosystem
cuixq Nov 27, 2023
53c3016
Merge branch 'google:main' into group
cuixq Nov 27, 2023
a23817f
addressing comments
cuixq Nov 15, 2023
a9a213e
lint
cuixq Nov 27, 2023
f414b44
fix window tests
cuixq Nov 28, 2023
a5cd81e
only check ecosystem for empty PackageDetails
cuixq Dec 8, 2023
f7dce24
Merge branch 'main' into group
cuixq Dec 8, 2023
11d7a28
Merge branch 'main' into group
cuixq Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion internal/ci/vulnerability_result_diff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package ci

import (
"reflect"

"github.com/google/osv-scanner/pkg/grouper"
"github.com/google/osv-scanner/pkg/models"
"golang.org/x/exp/slices"
Expand All @@ -26,7 +28,7 @@ func DiffVulnerabilityResults(oldRes, newRes models.VulnerabilityResults) models
resultPS := &result.Results[len(result.Results)-1]
for _, pv := range ps.Packages {
pkgs := oldRes.Results[sourceIdx].Packages
pkgIdx := slices.IndexFunc(pkgs, func(elem models.PackageVulns) bool { return elem.Package == pv.Package })
pkgIdx := slices.IndexFunc(pkgs, func(elem models.PackageVulns) bool { return reflect.DeepEqual(elem.Package, pv.Package) })
cuixq marked this conversation as resolved.
Show resolved Hide resolved
if pkgIdx == -1 {
// Newly introduced package, so all results for this package are going to be new, add everything for this package
resultPS.Packages = append(resultPS.Packages, pv)
Expand Down
6 changes: 5 additions & 1 deletion internal/output/table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,11 @@ func tableBuilderInner(vulnResult *models.VulnerabilityResults, addStyling bool,
outputRow = append(outputRow, "GIT", pkgCommitStr, pkgCommitStr)
shouldMerge = true
} else {
outputRow = append(outputRow, pkg.Package.Ecosystem, pkg.Package.Name, pkg.Package.Version)
name := pkg.Package.Name
if len(pkg.DepGroups) > 0 {
name += fmt.Sprintf(" (%s)", strings.Join(pkg.DepGroups, ","))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, can we perhaps be more opinionated here in the table output, and just output e.g. "(dev)" in the cases where it is a dev dependency?

Users may not care about optional etc most of the time, and it would just clutter this table. If they do want the exact group details, they can use the JSON output.

We can define perhaps some kind of per-ecosystem function which just does:

type Ecosystem interface {
  IsDevGroup(groups []string) bool
}

or something similar per ecosystem?

@another-rex @G-Rath wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ecosystem is now a string can we still make it an interface?

Or, we can have something like this?

func (s Ecosystem) IsDevGroup(groups []string) bool {
  switch s {
  case CargoEcosystem: 
     ...
  case NpmEcocystem:
    ...
  }
  ...
  return false;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree we should just stick to a single output in the table output, but I think we should do it with a separate column.

I'm thinking "prod" column with a checkmark if it's a production dependency, and empty if not a production dep. This way it makes it applicable to all the other not "dev" named dependencies (e.g. optional in npm).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re column, I think the reason we went with a "(dev)" marker for now is that it because the column becomes redundant whenever we do/support any kind of filtering, and makes things inconsistent.

This is also somethign we can change at any time, given that there's no stability guarantees for the table/human readable output.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a method on Ecosystem to check if there is dev group in the provided groups.

}
outputRow = append(outputRow, pkg.Package.Ecosystem, name, pkg.Package.Version)
}

outputRow = append(outputRow, source.Path)
Expand Down
3 changes: 2 additions & 1 deletion pkg/lockfile/dpkg-status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bufio"
"fmt"
"os"
"reflect"
"sort"
"strings"

Expand Down Expand Up @@ -143,7 +144,7 @@ func (e DpkgStatusExtractor) Extract(f DepFile) ([]PackageDetails, error) {

// PackageDetails does not contain any field that represent a "not installed" state
// To manage this state and avoid false positives, empty struct means "not installed" so skip it
if (PackageDetails{}) == pkg {
if reflect.DeepEqual(PackageDetails{}, pkg) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my understanding is that pulling in reflect can be a big deal in Go, and at at least in the past with some methods (which tbh probably don't include DeepEqual) could trigger sizeable deoptimizations.

I don't think this check is anything special and very internal so could be easily replaced with something like returning a pointer (so that we can then return nil), returning an error that we can eat here, or setting the Name to something like <not installed>.

I think that's probably worth doing either way so that we're not as dependent on the whole structure of PackageDetails

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified the code here to check each field in PackageDetails and if they are all "empty". wdty?

continue
}

Expand Down
10 changes: 10 additions & 0 deletions pkg/lockfile/fixtures/maven/with-scope.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<project>
<dependencies>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>4.12</version>
<scope>test</scope>
</dependency>
</dependencies>
</project>
22 changes: 22 additions & 0 deletions pkg/lockfile/fixtures/npm/optional-package.v1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
{
"requires": true,
"lockfileVersion": 1,
"dependencies": {
"wrappy": {
"version": "1.0.2",
"resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.2.tgz",
"integrity": "sha1-tSQ9jz7BqjXxNkYFvA0QNuMKtp8=",
"dev": true,
"optional": true
},
"supports-color": {
"version": "5.5.0",
"resolved": "https://registry.npmjs.org/supports-color/-/supports-color-5.5.0.tgz",
"integrity": "sha512-QjVjwdXIt408MIiAqCX4oUKsgU2EqAGzs2Ppkm4aQYbjm+ZEWEcW4SfFNTr4uMNZma0ey4f5lgLrkB0aX0QMow==",
"optional": true,
"requires": {
"has-flag": "^3.0.0"
}
}
}
}
30 changes: 30 additions & 0 deletions pkg/lockfile/fixtures/npm/optional-package.v2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{
"name": "my-library",
"lockfileVersion": 2,
"requires": true,
"packages": {
"": {
"dependencies": {},
"devDependencies": { "wrappy": "^1.0.0" }
},
"node_modules/wrappy": {
"version": "1.0.2",
"resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.2.tgz",
"integrity": "sha1-tSQ9jz7BqjXxNkYFvA0QNuMKtp8=",
"optional": true
},
"node_modules/supports-color": {
"version": "5.5.0",
"resolved": "https://registry.npmjs.org/supports-color/-/supports-color-5.5.0.tgz",
"integrity": "sha512-QjVjwdXIt408MIiAqCX4oUKsgU2EqAGzs2Ppkm4aQYbjm+ZEWEcW4SfFNTr4uMNZma0ey4f5lgLrkB0aX0QMow==",
"devOptional": true,
"dependencies": {
"has-flag": "^3.0.0"
},
"engines": {
"node": ">=4"
}
}
},
"dependencies": {}
}
15 changes: 15 additions & 0 deletions pkg/lockfile/fixtures/poetry/optional-package.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
[[package]]
name = "numpy"
version = "1.23.3"
description = "NumPy is the fundamental package for array computing with Python."
category = "main"
optional = true
python-versions = ">=3.8"

[metadata]
lock-version = "1.1"
python-versions = "^3.8"
content-hash = "399777887f0c3171cbc3fc8a8e350d0fca4d882cf126657f60ec83872572ed44"

[metadata.files]
numpy = []
3 changes: 2 additions & 1 deletion pkg/lockfile/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package lockfile_test
import (
"errors"
"fmt"
"reflect"
"strings"
"testing"

Expand Down Expand Up @@ -48,7 +49,7 @@ func hasPackage(t *testing.T, packages []lockfile.PackageDetails, pkg lockfile.P
t.Helper()

for _, details := range packages {
if details == pkg {
if reflect.DeepEqual(details, pkg) {
return true
}
}
Expand Down
1 change: 1 addition & 0 deletions pkg/lockfile/parse-composer-lock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ func (e ComposerLockExtractor) Extract(f DepFile) ([]PackageDetails, error) {
Commit: composerPackage.Dist.Reference,
Ecosystem: ComposerEcosystem,
CompareAs: ComposerEcosystem,
DepGroups: []string{"dev"},
})
}

Expand Down
2 changes: 2 additions & 0 deletions pkg/lockfile/parse-composer-lock_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ func TestParseComposerLock_OnePackageDev(t *testing.T) {
Commit: "4c115873c86ad5bd0ac6d962db70ca53bf8fb874",
Ecosystem: lockfile.ComposerEcosystem,
CompareAs: lockfile.ComposerEcosystem,
DepGroups: []string{"dev"},
},
})
}
Expand Down Expand Up @@ -152,6 +153,7 @@ func TestParseComposerLock_TwoPackages(t *testing.T) {
Commit: "11336f6f84e16a720dae9d8e6ed5019efa85a0f9",
Ecosystem: lockfile.ComposerEcosystem,
CompareAs: lockfile.ComposerEcosystem,
DepGroups: []string{"dev"},
},
})
}
Expand Down
10 changes: 10 additions & 0 deletions pkg/lockfile/parse-conan-lock-v2_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ func TestParseConanLock_v2_OnePackage(t *testing.T) {
Version: "1.2.11",
Ecosystem: lockfile.ConanEcosystem,
CompareAs: lockfile.ConanEcosystem,
DepGroups: []string{"requires"},
},
})
}
Expand All @@ -71,6 +72,7 @@ func TestParseConanLock_v2_NoName(t *testing.T) {
Version: "1.2.11",
Ecosystem: lockfile.ConanEcosystem,
CompareAs: lockfile.ConanEcosystem,
DepGroups: []string{"requires"},
},
})
}
Expand All @@ -90,12 +92,14 @@ func TestParseConanLock_v2_TwoPackages(t *testing.T) {
Version: "1.2.11",
Ecosystem: lockfile.ConanEcosystem,
CompareAs: lockfile.ConanEcosystem,
DepGroups: []string{"requires"},
},
{
Name: "bzip2",
Version: "1.0.8",
Ecosystem: lockfile.ConanEcosystem,
CompareAs: lockfile.ConanEcosystem,
DepGroups: []string{"requires"},
},
})
}
Expand All @@ -115,30 +119,35 @@ func TestParseConanLock_v2_NestedDependencies(t *testing.T) {
Version: "1.2.13",
Ecosystem: lockfile.ConanEcosystem,
CompareAs: lockfile.ConanEcosystem,
DepGroups: []string{"requires"},
},
{
Name: "bzip2",
Version: "1.0.8",
Ecosystem: lockfile.ConanEcosystem,
CompareAs: lockfile.ConanEcosystem,
DepGroups: []string{"requires"},
},
{
Name: "freetype",
Version: "2.12.1",
Ecosystem: lockfile.ConanEcosystem,
CompareAs: lockfile.ConanEcosystem,
DepGroups: []string{"requires"},
},
{
Name: "libpng",
Version: "1.6.39",
Ecosystem: lockfile.ConanEcosystem,
CompareAs: lockfile.ConanEcosystem,
DepGroups: []string{"requires"},
},
{
Name: "brotli",
Version: "1.0.9",
Ecosystem: lockfile.ConanEcosystem,
CompareAs: lockfile.ConanEcosystem,
DepGroups: []string{"requires"},
},
})
}
Expand All @@ -158,6 +167,7 @@ func TestParseConanLock_v2_OnePackageDev(t *testing.T) {
Version: "1.11.1",
Ecosystem: lockfile.ConanEcosystem,
CompareAs: lockfile.ConanEcosystem,
DepGroups: []string{"build-requires"},
},
})
}
9 changes: 5 additions & 4 deletions pkg/lockfile/parse-conan-lock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ func parseConanV1Lock(lockfile ConanLockFile) []PackageDetails {
return packages
}

func parseConanRequires(packages *[]PackageDetails, requires []string) {
func parseConanRequires(packages *[]PackageDetails, requires []string, group string) {
for _, ref := range requires {
reference := parseConanRenference(ref)
// skip entries with no name, they are most likely consumer's conanfiles
Expand All @@ -146,6 +146,7 @@ func parseConanRequires(packages *[]PackageDetails, requires []string) {
Version: reference.Version,
Ecosystem: ConanEcosystem,
CompareAs: ConanEcosystem,
DepGroups: []string{group},
})
}
}
Expand All @@ -157,9 +158,9 @@ func parseConanV2Lock(lockfile ConanLockFile) []PackageDetails {
uint64(len(lockfile.Requires))+uint64(len(lockfile.BuildRequires))+uint64(len(lockfile.PythonRequires)),
)

parseConanRequires(&packages, lockfile.Requires)
parseConanRequires(&packages, lockfile.BuildRequires)
parseConanRequires(&packages, lockfile.PythonRequires)
parseConanRequires(&packages, lockfile.Requires, "requires")
parseConanRequires(&packages, lockfile.BuildRequires, "build-requires")
parseConanRequires(&packages, lockfile.PythonRequires, "python-requires")

return packages
}
Expand Down
15 changes: 12 additions & 3 deletions pkg/lockfile/parse-maven-lock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"os"
"path/filepath"
"strings"

"github.com/google/osv-scanner/internal/cachedregexp"
)
Expand All @@ -14,6 +15,7 @@ type MavenLockDependency struct {
GroupID string `xml:"groupId"`
ArtifactID string `xml:"artifactId"`
Version string `xml:"version"`
Scope string `xml:"scope"`
}

func (mld MavenLockDependency) parseResolvedVersion(version string) string {
Expand Down Expand Up @@ -121,24 +123,31 @@ func (e MavenLockExtractor) Extract(f DepFile) ([]PackageDetails, error) {
for _, lockPackage := range parsedLockfile.Dependencies {
finalName := lockPackage.GroupID + ":" + lockPackage.ArtifactID

details[finalName] = PackageDetails{
pkgDetails := PackageDetails{
Name: finalName,
Version: lockPackage.ResolveVersion(*parsedLockfile),
Ecosystem: MavenEcosystem,
CompareAs: MavenEcosystem,
}
if strings.TrimSpace(lockPackage.Scope) != "" {
pkgDetails.DepGroups = append(pkgDetails.DepGroups, lockPackage.Scope)
}
details[finalName] = pkgDetails
}

// managed dependencies take precedent over standard dependencies
for _, lockPackage := range parsedLockfile.ManagedDependencies {
finalName := lockPackage.GroupID + ":" + lockPackage.ArtifactID

details[finalName] = PackageDetails{
pkgDetails := PackageDetails{
Name: finalName,
Version: lockPackage.ResolveVersion(*parsedLockfile),
Ecosystem: MavenEcosystem,
CompareAs: MavenEcosystem,
}
if strings.TrimSpace(lockPackage.Scope) != "" {
pkgDetails.DepGroups = append(pkgDetails.DepGroups, lockPackage.Scope)
}
details[finalName] = pkgDetails
}

return pkgDetailsMapToSlice(details), nil
Expand Down
20 changes: 20 additions & 0 deletions pkg/lockfile/parse-maven-lock_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,3 +290,23 @@ func TestMavenLockDependency_ResolveVersion(t *testing.T) {
})
}
}

func TestParseMavenLock_WithScope(t *testing.T) {
t.Parallel()

packages, err := lockfile.ParseMavenLock("fixtures/maven/with-scope.xml")

if err != nil {
t.Errorf("Got unexpected error: %v", err)
}

expectPackages(t, packages, []lockfile.PackageDetails{
{
Name: "junit:junit",
Version: "4.12",
Ecosystem: lockfile.MavenEcosystem,
CompareAs: lockfile.MavenEcosystem,
DepGroups: []string{"test"},
},
})
}
Loading