Skip to content

Commit

Permalink
Refactor models (#510)
Browse files Browse the repository at this point in the history
Pulled refactor out to a separate PR as suggested here: #505 

In this PR:
- Refactored models package to no longer depend on lockfile
- Moved functions that required lockfile types into separate internal
package under `utility/vulns`
- Copied over OSV url from `osv` package to avoid dependency on `osv`
from `output`/`reporter`

I went through and graphed out the dependencies, 

- `models` no longer depends on any internal dependencies
- `lockfile` doesn't depend on any internal dependencies apart from
`cacheexp`
- `osv` brings in both `lockfiles` and `models`. This is probably the
most problematic one. Ideally `osv` should only be referencing `models`,
but it'll be a breaking change to change this API. This is one of the
main things we should change for V2 (related to #442) (TODO: Make
separate issue for this
- `reporter` depends on the internal `output`, which depends on `osv`.
This dependency causes `lockfile` to be pulled into everywhere that
needs to print out anything. This can be decoupled without too much
trouble by copying in the OSV url. I did so in this PR.
- `config` package references `reporter`, with the above change it also
no longer depends on `lockfile`.
  • Loading branch information
another-rex authored Aug 28, 2023
1 parent e1bb554 commit 5a316f5
Show file tree
Hide file tree
Showing 10 changed files with 1,015 additions and 986 deletions.
3 changes: 2 additions & 1 deletion internal/local/zip.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"path"
"strings"

"github.com/google/osv-scanner/internal/utility/vulns"
"github.com/google/osv-scanner/pkg/lockfile"
"github.com/google/osv-scanner/pkg/models"
"github.com/google/osv-scanner/pkg/osv"
Expand Down Expand Up @@ -235,7 +236,7 @@ func (db *ZipDB) VulnerabilitiesAffectingPackage(pkg lockfile.PackageDetails) mo
var vulnerabilities models.Vulnerabilities

for _, vulnerability := range db.Vulnerabilities(false) {
if vulnerability.IsAffected(pkg) && !vulnerabilities.Includes(vulnerability) {
if vulns.IsAffected(vulnerability, pkg) && !vulns.Include(vulnerabilities, vulnerability) {
vulnerabilities = append(vulnerabilities, vulnerability)
}
}
Expand Down
9 changes: 6 additions & 3 deletions internal/output/table.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,15 @@ import (
v3_metric "github.com/goark/go-cvss/v3/metric"

"github.com/google/osv-scanner/pkg/models"
"github.com/google/osv-scanner/pkg/osv"

"github.com/jedib0t/go-pretty/v6/table"
"github.com/jedib0t/go-pretty/v6/text"
)

// OSVBaseVulnerabilityURL is the base URL for detailed vulnerability views.
// Copied in from osv package to avoid referencing the osv package unnecessarily
const OSVBaseVulnerabilityURL = "https://osv.dev/"

// PrintTableResults prints the osv scan results into a human friendly table.
func PrintTableResults(vulnResult *models.VulnerabilityResults, outputWriter io.Writer, terminalWidth int) {
outputTable := table.NewWriter()
Expand Down Expand Up @@ -96,9 +99,9 @@ func tableBuilderInner(vulnResult *models.VulnerabilityResults, addStyling bool,

for _, vuln := range group.IDs {
if addStyling {
links = append(links, osv.BaseVulnerabilityURL+text.Bold.EscapeSeq()+vuln+text.Reset.EscapeSeq())
links = append(links, OSVBaseVulnerabilityURL+text.Bold.EscapeSeq()+vuln+text.Reset.EscapeSeq())
} else {
links = append(links, osv.BaseVulnerabilityURL+vuln)
links = append(links, OSVBaseVulnerabilityURL+vuln)
}
}

Expand Down
20 changes: 20 additions & 0 deletions internal/utility/vulns/vulnerabilities.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package vulns

import "github.com/google/osv-scanner/pkg/models"

func Include(vs models.Vulnerabilities, vulnerability models.Vulnerability) bool {
for _, vuln := range vs {
if vuln.ID == vulnerability.ID {
return true
}

if isAliasOf(vuln, vulnerability) {
return true
}
if isAliasOf(vulnerability, vuln) {
return true
}
}

return false
}
129 changes: 129 additions & 0 deletions internal/utility/vulns/vulnerabilities_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
package vulns_test

import (
"testing"

"github.com/google/osv-scanner/internal/utility/vulns"
"github.com/google/osv-scanner/pkg/models"
)

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

type args struct {
osv models.Vulnerability
}
tests := []struct {
name string
vs models.Vulnerabilities
args args
want bool
}{
{
name: "",
vs: models.Vulnerabilities{
models.Vulnerability{
ID: "GHSA-1",
Aliases: []string{},
},
},
args: args{
osv: models.Vulnerability{
ID: "GHSA-2",
Aliases: []string{},
},
},
want: false,
},
{
name: "",
vs: models.Vulnerabilities{
models.Vulnerability{
ID: "GHSA-1",
Aliases: []string{},
},
},
args: args{
osv: models.Vulnerability{
ID: "GHSA-1",
Aliases: []string{},
},
},
want: true,
},
{
name: "",
vs: models.Vulnerabilities{
models.Vulnerability{
ID: "GHSA-1",
Aliases: []string{"GHSA-2"},
},
},
args: args{
osv: models.Vulnerability{
ID: "GHSA-2",
Aliases: []string{},
},
},
want: true,
},
{
name: "",
vs: models.Vulnerabilities{
models.Vulnerability{
ID: "GHSA-1",
Aliases: []string{},
},
},
args: args{
osv: models.Vulnerability{
ID: "GHSA-2",
Aliases: []string{"GHSA-1"},
},
},
want: true,
},
{
name: "",
vs: models.Vulnerabilities{
models.Vulnerability{
ID: "GHSA-1",
Aliases: []string{"CVE-1"},
},
},
args: args{
osv: models.Vulnerability{
ID: "GHSA-2",
Aliases: []string{"CVE-1"},
},
},
want: true,
},
{
name: "",
vs: models.Vulnerabilities{
models.Vulnerability{
ID: "GHSA-1",
Aliases: []string{"CVE-2"},
},
},
args: args{
osv: models.Vulnerability{
ID: "GHSA-2",
Aliases: []string{"CVE-2"},
},
},
want: true,
},
}
for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

if got := vulns.Include(tt.vs, tt.args.osv); got != tt.want {
t.Errorf("Includes() = %v, want %v", got, tt.want)
}
})
}
}
152 changes: 152 additions & 0 deletions internal/utility/vulns/vulnerability.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
package vulns

import (
"fmt"
"os"
"sort"

"github.com/google/osv-scanner/internal/semantic"
"github.com/google/osv-scanner/pkg/lockfile"
"github.com/google/osv-scanner/pkg/models"
"golang.org/x/exp/slices"
)

func eventVersion(e models.Event) string {
if e.Introduced != "" {
return e.Introduced
}

if e.Fixed != "" {
return e.Fixed
}

if e.Limit != "" {
return e.Limit
}

if e.LastAffected != "" {
return e.LastAffected
}

return ""
}

func rangeContainsVersion(ar models.Range, pkg lockfile.PackageDetails) bool {
if ar.Type != models.RangeEcosystem && ar.Type != models.RangeSemVer {
return false
}
// todo: we should probably warn here
if len(ar.Events) == 0 {
return false
}

vp := semantic.MustParse(pkg.Version, pkg.CompareAs)

sort.Slice(ar.Events, func(i, j int) bool {
a := ar.Events[i]
b := ar.Events[j]

if a.Introduced == "0" {
return true
}

if b.Introduced == "0" {
return false
}

return semantic.MustParse(eventVersion(a), pkg.CompareAs).CompareStr(eventVersion(b)) < 0
})

var affected bool
for _, e := range ar.Events {
if affected {
if e.Fixed != "" {
affected = vp.CompareStr(e.Fixed) < 0
} else if e.LastAffected != "" {
affected = e.LastAffected == pkg.Version || vp.CompareStr(e.LastAffected) <= 0
}
} else if e.Introduced != "" {
affected = e.Introduced == "0" || vp.CompareStr(e.Introduced) >= 0
}
}

return affected
}

// rangeAffectsVersion checks if the given version is within the range
// specified by the events of any "Ecosystem" or "Semver" type ranges
func rangeAffectsVersion(a []models.Range, pkg lockfile.PackageDetails) bool {
for _, r := range a {
if r.Type != models.RangeEcosystem && r.Type != models.RangeSemVer {
return false
}
if rangeContainsVersion(r, pkg) {
return true
}
}

return false
}

func isAliasOfID(v models.Vulnerability, id string) bool {
for _, alias := range v.Aliases {
if alias == id {
return true
}
}

return false
}

func isAliasOf(v models.Vulnerability, vulnerability models.Vulnerability) bool {
for _, alias := range vulnerability.Aliases {
if v.ID == alias || isAliasOfID(v, alias) {
return true
}
}

return false
}

func AffectsEcosystem(v models.Vulnerability, ecosystem lockfile.Ecosystem) bool {
for _, affected := range v.Affected {
if string(affected.Package.Ecosystem) == string(ecosystem) {
return true
}
}

return false
}

func IsAffected(v models.Vulnerability, pkg lockfile.PackageDetails) bool {
for _, affected := range v.Affected {
if string(affected.Package.Ecosystem) == string(pkg.Ecosystem) &&
affected.Package.Name == pkg.Name {
if len(affected.Ranges) == 0 && len(affected.Versions) == 0 {
_, _ = fmt.Fprintf(
os.Stderr,
"%s does not have any ranges or versions - this is probably a mistake!\n",
v.ID,
)

continue
}

if slices.Contains(affected.Versions, pkg.Version) {
return true
}

if rangeAffectsVersion(affected.Ranges, pkg) {
return true
}

// if a package does not have a version, assume it is vulnerable
// as false positives are better than false negatives here
if pkg.Version == "" {
return true
}
}
}

return false
}
Loading

0 comments on commit 5a316f5

Please sign in to comment.