Repository: G-Rath/osv-detector
Pull request #56 (2022-03-11 02:43:49)

fix: normalize names of python packages to favor false positives over false negatives
The comments should say it all - ultimately I realised that `pip-compile` is probably why all the `requirements.txt` I have locally are using lowercase package names, and that pip probably just accepts either or and normailizes names internally.

Ultimately the OSV spec says that the name for packages in the PyPip ecosystem should be normalized, so let's normalize the names on both sides and call it a day for the time being.


Patch:
diff --git a/detector/database/osv.go b/detector/database/osv.go
index bff00e9..b2961c4 100644
--- a/detector/database/osv.go
+++ b/detector/database/osv.go
@@ -4,7 +4,10 @@ import (
 	"fmt"
 	"os"
 	"osv-detector/detector"
+	"osv-detector/detector/parsers"
 	"osv-detector/detector/semver"
+	"regexp"
+	"strings"
 	"time"
 )
 
@@ -23,6 +26,25 @@ type Package struct {
 	Ecosystem Ecosystem `json:"ecosystem"`
 }
 
+// NormalizedName ensures that the package name is normalized based on ecosystem
+// in accordance to the OSV specification.
+//
+// This is required because currently both GitHub and Pip seem to be a bit
+// inconsistent in their package name handling, so we normalize them
+// to be on the safe side.
+//
+// In the future, it's hoped that this can be improved.
+func (p Package) NormalizedName() string {
+	if p.Ecosystem != parsers.PipEcosystem {
+		return p.Name
+	}
+
+	// per https://www.python.org/dev/peps/pep-0503/#normalized-names
+	name := regexp.MustCompile(`[-_.]+`).ReplaceAllString(p.Name, "-")
+
+	return strings.ToLower(name)
+}
+
 type RangeEvent struct {
 	Introduced string `json:"introduced,omitempty"`
 	Fixed      string `json:"fixed,omitempty"`
@@ -122,7 +144,8 @@ func (osv *OSV) IsAffected(pkg detector.PackageDetails) bool {
 	}
 
 	for _, affected := range osv.Affected {
-		if affected.Package.Ecosystem == pkg.Ecosystem && affected.Package.Name == pkg.Name {
+		if affected.Package.Ecosystem == pkg.Ecosystem &&
+			affected.Package.NormalizedName() == pkg.Name {
 			if len(affected.Ranges) == 0 {
 				_, _ = fmt.Fprintf(
 					os.Stderr,
diff --git a/detector/parsers/fixtures/pip/non-normalized-names.txt b/detector/parsers/fixtures/pip/non-normalized-names.txt
new file mode 100644
index 0000000..a0a6d8e
--- /dev/null
+++ b/detector/parsers/fixtures/pip/non-normalized-names.txt
@@ -0,0 +1,9 @@
+zope.interface==5.4.0
+    # via
+    #   scrapy
+    #   twisted
+
+Pillow==1.0.0
+
+Twisted[http2]==20.3.0
+    # via scrapy
diff --git a/detector/parsers/parse-requirements-txt.go b/detector/parsers/parse-requirements-txt.go
index 288bb75..bbffe62 100644
--- a/detector/parsers/parse-requirements-txt.go
+++ b/detector/parsers/parse-requirements-txt.go
@@ -45,14 +45,30 @@ func parseLine(line string) PackageDetails {
 	}
 
 	return PackageDetails{
-		Name:      cleanupRequirementName(name),
+		Name:      normalizedRequirementName(name),
 		Version:   version,
 		Ecosystem: PipEcosystem,
 	}
 }
 
-func cleanupRequirementName(name string) string {
-	return strings.Split(name, "[")[0]
+// normalizedName ensures that the package name is normalized per PEP-0503
+// and then removing "added support" syntax if present.
+//
+// This is done to ensure we don't miss any advisories, as while the OSV
+// specification says that the normalized name should be used for advisories,
+// that's not the case currently in our databases, _and_ Pip itself supports
+// non-normalized names in the requirements.txt, so we need to normalize
+// on both sides to ensure we don't have false negatives.
+//
+// It's possible that this will cause some false positives, but that is better
+// than false negatives, and can be dealt with when/if it actually happens.
+func normalizedRequirementName(name string) string {
+	// per https://www.python.org/dev/peps/pep-0503/#normalized-names
+	name = regexp.MustCompile(`[-_.]+`).ReplaceAllString(name, "-")
+	name = strings.ToLower(name)
+	name = strings.Split(name, "[")[0]
+
+	return name
 }
 
 func removeComments(line string) string {


Test patch:
diff --git a/detector/database/osv_test.go b/detector/database/osv_test.go
index d03db3a..a6fa251 100644
--- a/detector/database/osv_test.go
+++ b/detector/database/osv_test.go
@@ -36,6 +36,61 @@ func buildEcosystemAffectsRange(events ...database.RangeEvent) database.AffectsR
 	return database.AffectsRange{Type: database.TypeEcosystem, Events: events}
 }
 
+func TestPackage_NormalizedName(t *testing.T) {
+	t.Parallel()
+
+	database.Package{
+		Name:      "",
+		Ecosystem: "",
+	}.NormalizedName()
+}
+
+func TestPackage_NormalizedName_PipEcosystem(t *testing.T) {
+	t.Parallel()
+
+	x := [][]string{
+		{"Pillow", "pillow"},
+		{"privacyIDEA", "privacyidea"},
+		{"Products.GenericSetup", "products-genericsetup"},
+	}
+
+	for _, strings := range x {
+		name := database.Package{Name: strings[0], Ecosystem: parsers.PipEcosystem}.NormalizedName()
+
+		if name != strings[1] {
+			t.Errorf(
+				"Expected package named %s to be normalized to %s but was normalized to %s instead",
+				strings[0],
+				strings[1],
+				name,
+			)
+		}
+	}
+}
+
+func TestPackage_NormalizedName_NotPipEcosystem(t *testing.T) {
+	t.Parallel()
+
+	x := []string{
+		"Proto",
+		"Pillow",
+		"privacyIDEA",
+		"Products.GenericSetup",
+	}
+
+	for _, na := range x {
+		name := database.Package{Name: na, Ecosystem: parsers.NpmEcosystem}.NormalizedName()
+
+		if name != na {
+			t.Errorf(
+				"Expected package named %s to be unchanged, but it was normalized to %s",
+				na,
+				name,
+			)
+		}
+	}
+}
+
 func TestOSV_AffectsEcosystem(t *testing.T) {
 	t.Parallel()
 
@@ -240,3 +295,46 @@ func TestOSV_IsAffected_AffectsWithEcosystem_MultipleAffected(t *testing.T) {
 		expectIsAffected(t, osv, v, false)
 	}
 }
+
+func TestOSV_IsAffected_AffectsWithEcosystem_PipNamesAreNormalised(t *testing.T) {
+	t.Parallel()
+
+	var osv database.OSV
+	var pkg detector.PackageDetails
+
+	osv = buildOSVWithAffected(
+		database.Affected{
+			Package: database.Package{Ecosystem: parsers.PipEcosystem, Name: "Pillow"},
+			Ranges: []database.AffectsRange{
+				buildEcosystemAffectsRange(
+					database.RangeEvent{Introduced: "0"},
+					database.RangeEvent{Fixed: "1"},
+				),
+			},
+		},
+	)
+
+	pkg = detector.PackageDetails{Name: "pillow", Version: "0.5", Ecosystem: parsers.PipEcosystem}
+
+	if !osv.IsAffected(pkg) {
+		t.Errorf("Expected OSV to normalize names of pip packages, but did not")
+	}
+
+	osv = buildOSVWithAffected(
+		database.Affected{
+			Package: database.Package{Ecosystem: parsers.NpmEcosystem, Name: "Pillow"},
+			Ranges: []database.AffectsRange{
+				buildEcosystemAffectsRange(
+					database.RangeEvent{Introduced: "0"},
+					database.RangeEvent{Fixed: "1"},
+				),
+			},
+		},
+	)
+
+	pkg = detector.PackageDetails{Name: "pillow", Version: "0.5", Ecosystem: parsers.NpmEcosystem}
+
+	if osv.IsAffected(pkg) {
+		t.Errorf("Expected OSV not to normalize names of non-pip packages, but it did")
+	}
+}
diff --git a/detector/parsers/parse-requirements-txt_test.go b/detector/parsers/parse-requirements-txt_test.go
index 23d0a27..eae8ce0 100644
--- a/detector/parsers/parse-requirements-txt_test.go
+++ b/detector/parsers/parse-requirements-txt_test.go
@@ -246,7 +246,7 @@ func TestParseRequirementsTxt_FileFormatExample(t *testing.T) {
 			Ecosystem: parsers.PipEcosystem,
 		},
 		{
-			Name:      "Mopidy-Dirble",
+			Name:      "mopidy-dirble",
 			Version:   "1.1",
 			Ecosystem: parsers.PipEcosystem,
 		},
@@ -280,3 +280,31 @@ func TestParseRequirementsTxt_WithAddedSupport(t *testing.T) {
 		},
 	})
 }
+
+func TestParseRequirementsTxt_NonNormalizedNames(t *testing.T) {
+	t.Parallel()
+
+	packages, err := parsers.ParseRequirementsTxt("fixtures/pip/non-normalized-names.txt")
+
+	if err != nil {
+		t.Errorf("Got unexpected error: %v", err)
+	}
+
+	expectPackages(t, packages, []parsers.PackageDetails{
+		{
+			Name:      "zope-interface",
+			Version:   "5.4.0",
+			Ecosystem: parsers.PipEcosystem,
+		},
+		{
+			Name:      "pillow",
+			Version:   "1.0.0",
+			Ecosystem: parsers.PipEcosystem,
+		},
+		{
+			Name:      "twisted",
+			Version:   "20.3.0",
+			Ecosystem: parsers.PipEcosystem,
+		},
+	})
+}