Repository: G-Rath/osv-detector
Pull request #60 (2022-03-11 21:24:48)

fix: improve description of OSVs to handle summary not being present + linking to non GH advisories
The output format is slowly getting better 😄 

Currently I'm applying truncation to just the "details" text as the summaries seem short enough and really this is meant to be a fallback now that we're using more than just the GH advisory database.

I have just realised a slight improvement for linking would be to check if the `aliases` contain any GHSA IDs - will do that in a follow-up PR.


Patch:
diff --git a/detector/database/osv.go b/detector/database/osv.go
index af8fda2..07d5ad5 100644
--- a/detector/database/osv.go
+++ b/detector/database/osv.go
@@ -9,6 +9,7 @@ import (
 	"regexp"
 	"strings"
 	"time"
+	"unicode"
 )
 
 type AffectsRangeType string
@@ -145,8 +146,62 @@ func (osv *OSV) AffectsEcosystem(ecosystem detector.Ecosystem) bool {
 	return false
 }
 
+// truncate ensures that the given string is shorter than the provided limit.
+//
+// If the string is longer than the limit, it's trimmed and suffixed with an ellipsis.
+// Ideally the string will be trimmed at the space that's closest to the limit to
+// preserve whole words; if a string has no spaces before the limit, it'll be forcefully truncated.
+func truncate(str string, limit int) string {
+	count := 0
+	truncateAt := -1
+
+	for i, c := range str {
+		if unicode.IsSpace(c) {
+			truncateAt = i
+		}
+
+		count++
+
+		if count >= limit {
+			// ideally we want to keep words whole when truncating,
+			// but if we can't find a space just truncate at the limit
+			if truncateAt == -1 {
+				truncateAt = limit
+			}
+
+			return str[:truncateAt] + "..."
+		}
+	}
+
+	return str
+}
+
+func (osv OSV) Describe() string {
+	description := osv.Summary
+
+	if description == "" {
+		description += truncate(osv.Details, 80)
+	}
+
+	if description == "" {
+		description += "(no details available)"
+	}
+
+	if link := osv.Link(); link != "" {
+		description += " (" + link + ")"
+	}
+
+	return description
+}
+
+// Link returns a URL to the advisory, if possible.
+// Otherwise, an empty string is returned
 func (osv *OSV) Link() string {
-	return "https://github.com/advisories/" + osv.ID
+	if strings.HasPrefix(osv.ID, "GHSA") {
+		return "https://github.com/advisories/" + osv.ID
+	}
+
+	return ""
 }
 
 func (osv *OSV) IsAffected(pkg detector.PackageDetails) bool {
diff --git a/main.go b/main.go
index aab45d4..77ce39f 100644
--- a/main.go
+++ b/main.go
@@ -69,10 +69,9 @@ func printVulnerabilities(db database.OSVDatabase, pkg detector.PackageDetails)
 
 	for _, vulnerability := range vulnerabilities {
 		fmt.Printf(
-			"  %s %s (%s)\n",
+			"  %s %s\n",
 			color.CyanString("%s:", vulnerability.ID),
-			vulnerability.Summary,
-			vulnerability.Link(),
+			vulnerability.Describe(),
 		)
 	}
 


Test patch:
diff --git a/detector/database/osv_test.go b/detector/database/osv_test.go
index 54e6e8c..7ecf90d 100644
--- a/detector/database/osv_test.go
+++ b/detector/database/osv_test.go
@@ -8,6 +8,14 @@ import (
 	"time"
 )
 
+func expectOSVDescription(t *testing.T, expected string, osv database.OSV) {
+	t.Helper()
+
+	if actual := osv.Describe(); actual != expected {
+		t.Errorf("Expected \"%s\" but got \"%s\"", expected, actual)
+	}
+}
+
 func expectIsAffected(t *testing.T, osv database.OSV, version string, expectAffected bool) {
 	t.Helper()
 
@@ -488,3 +496,65 @@ func TestOSV_IsAffected_OnlyVersions(t *testing.T) {
 	expectIsAffected(t, osv, "1.0.0-beta1", false)
 	expectIsAffected(t, osv, "1.1.0", false)
 }
+
+func TestOSV_Describe_Text(t *testing.T) {
+	t.Parallel()
+
+	// use the summary of the advisory if present
+	expectOSVDescription(t, "This is a vulnerability!", database.OSV{Summary: "This is a vulnerability!"})
+
+	// otherwise, use the details of the advisory
+	expectOSVDescription(t, "It's very bad!", database.OSV{Details: "It's very bad!"})
+
+	// prefer the summary over details, as the former should be more concise
+	expectOSVDescription(t,
+		"This is a vulnerability!",
+		database.OSV{Summary: "This is a vulnerability!", Details: "It's very bad!"},
+	)
+
+	// advisories from GitHub should have a link included with their description
+	expectOSVDescription(t,
+		"This is a vulnerability! (https://github.com/advisories/GHSA-1)",
+		database.OSV{Summary: "This is a vulnerability!", ID: "GHSA-1"},
+	)
+
+	// if none of those are present, say that there are no details available
+	expectOSVDescription(t, "(no details available)", database.OSV{})
+}
+
+func TestOSV_Describe_Truncation(t *testing.T) {
+	t.Parallel()
+
+	// long details text should be truncated after 80 characters, based on words
+	expectOSVDescription(t,
+		"sqlparse is a non-validating SQL parser module for Python. In sqlparse versions...",
+		database.OSV{
+			Details: "sqlparse is a non-validating SQL parser module for Python. In sqlparse versions 0.4.0 and 0.4.1 there is a regular Expression Denial of Service in sqlparse vulnerability.",
+		},
+	)
+
+	// if a link is present, that isn't included in the truncating
+	expectOSVDescription(t,
+		"sqlparse is a non-validating SQL parser module for Python. In sqlparse versions... (https://github.com/advisories/GHSA-1)",
+		database.OSV{
+			Details: "sqlparse is a non-validating SQL parser module for Python. In sqlparse versions 0.4.0 and 0.4.1 there is a regular Expression Denial of Service in sqlparse vulnerability.",
+			ID:      "GHSA-1",
+		},
+	)
+
+	// long continuous text without any spaces before the limit should be forcefully truncated
+	expectOSVDescription(t,
+		"nannannannannannannannannannannannannannannannannannannannannannannannannannanna...",
+		database.OSV{
+			Details: "nannannannannannannannannannannannannannannannannannannannannannannannannannannannanan batman!",
+		},
+	)
+
+	// truncation shouldn't be applied to the summary text (as it should be short already)
+	expectOSVDescription(t,
+		"This is a vulnerability! It's very serious, and should not be taken lightly or bad things could happen!",
+		database.OSV{
+			Summary: "This is a vulnerability! It's very serious, and should not be taken lightly or bad things could happen!",
+		},
+	)
+}