Fix panic in requirements.txt parsing (#834)

* Stable sort for pipfile.lock parsing

Signed-off-by: Dan Luhring <dan+github@luhrings.com>

* Adjust python parsing tests to use go-cmp

Signed-off-by: Dan Luhring <dan+github@luhrings.com>

* Add failing cases for requirements.txt parsing

Signed-off-by: Dan Luhring <dan+github@luhrings.com>

* Fix failing cases for requirements.txt parsing

Signed-off-by: Dan Luhring <dan+github@luhrings.com>

* Refactor parseRequirementsTxt

Signed-off-by: Dan Luhring <dan+github@luhrings.com>

* Fix static-analysis failure

Signed-off-by: Dan Luhring <dan+github@luhrings.com>

* Fix comment

Signed-off-by: Dan Luhring <dan+github@luhrings.com>

syft/pkg/cataloger/python/parse_pipfile_lock.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"sort"
 	"strings"
 
 	"github.com/anchore/syft/syft/artifact"
@@ -60,5 +61,10 @@ func parsePipfileLock(_ string, reader io.Reader) ([]*pkg.Package, []artifact.Re
 		}
 	}
 
+	// Without sorting the packages slice, the order of packages will be unstable, due to ranging over a map.
+	sort.Slice(packages, func(i, j int) bool {
+		return packages[i].String() < packages[j].String()
+	})
+
 	return packages, nil, nil
 }

syft/pkg/cataloger/python/parse_pipfile_lock_test.go

@@ -4,36 +4,39 @@ import (
 	"os"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+
 	"github.com/anchore/syft/syft/pkg"
 )
 
 func TestParsePipFileLock(t *testing.T) {
-	expected := map[string]pkg.Package{
+	expected := []*pkg.Package{
-		"aio-pika": {
+		{
 			Name:     "aio-pika",
 			Version:  "6.8.0",
 			Language: pkg.Python,
 			Type:     pkg.PythonPkg,
 		},
-		"aiodns": {
+		{
 			Name:     "aiodns",
 			Version:  "2.0.0",
 			Language: pkg.Python,
 			Type:     pkg.PythonPkg,
 		},
-		"aiohttp": {
+		{
 			Name:     "aiohttp",
 			Version:  "3.7.4.post0",
 			Language: pkg.Python,
 			Type:     pkg.PythonPkg,
 		},
-		"aiohttp-jinja2": {
+		{
 			Name:     "aiohttp-jinja2",
 			Version:  "1.4.2",
 			Language: pkg.Python,
 			Type:     pkg.PythonPkg,
 		},
 	}
+
 	fixture, err := os.Open("test-fixtures/pipfile-lock/Pipfile.lock")
 	if err != nil {
 		t.Fatalf("failed to open fixture: %+v", err)
@@ -45,6 +48,7 @@ func TestParsePipFileLock(t *testing.T) {
 		t.Fatalf("failed to parse requirements: %+v", err)
 	}
 
-	assertPackagesEqual(t, actual, expected)
+	if diff := cmp.Diff(expected, actual, cmp.AllowUnexported(pkg.Package{})); diff != "" {
-
+		t.Errorf("unexpected result from parsing (-expected +actual)\n%s", diff)
+	}
 }

syft/pkg/cataloger/python/parse_requirements.go

@@ -22,26 +22,26 @@ func parseRequirementsTxt(_ string, reader io.Reader) ([]*pkg.Package, []artifac
 	scanner := bufio.NewScanner(reader)
 	for scanner.Scan() {
 		line := scanner.Text()
+		line = trimRequirementsTxtLine(line)
 
-		line = strings.TrimRight(line, "\n")
+		if line == "" {
-
+			// nothing to parse on this line
-		switch {
-		case strings.HasPrefix(line, "#"):
-			// commented out line, skip
 			continue
-		case strings.HasPrefix(line, "-e"):
+		}
+
+		if strings.HasPrefix(line, "-e") {
 			// editable packages aren't parsed (yet)
 			continue
-		case len(strings.Split(line, "==")) < 2:
+		}
-			// a package without a version, or a range (unpinned) which
+
-			// does not tell us exactly what will be installed
+		if !strings.Contains(line, "==") {
-			// XXX only needed if we want to log this, otherwise the next case catches it
+			// a package without a version, or a range (unpinned) which does not tell us
+			// exactly what will be installed.
 			continue
-		case len(strings.Split(line, "==")) == 2:
+		}
-			// remove comments if present
+
-			uncommented := removeTrailingComment(line)
 		// parse a new requirement
-			parts := strings.Split(uncommented, "==")
+		parts := strings.Split(line, "==")
 		name := strings.TrimSpace(parts[0])
 		version := strings.TrimSpace(parts[1])
 		packages = append(packages, &pkg.Package{
@@ -50,9 +50,6 @@ func parseRequirementsTxt(_ string, reader io.Reader) ([]*pkg.Package, []artifac
 			Language: pkg.Python,
 			Type:     pkg.PythonPkg,
 		})
-		default:
-			continue
-		}
 	}
 
 	if err := scanner.Err(); err != nil {
@@ -62,16 +59,37 @@ func parseRequirementsTxt(_ string, reader io.Reader) ([]*pkg.Package, []artifac
 	return packages, nil, nil
 }
 
+// trimRequirementsTxtLine removes content from the given requirements.txt line
+// that should not be considered for parsing.
+func trimRequirementsTxtLine(line string) string {
+	line = strings.TrimSpace(line)
+	line = removeTrailingComment(line)
+	line = removeEnvironmentMarkers(line)
+
+	return line
+}
+
 // removeTrailingComment takes a requirements.txt line and strips off comment strings.
 func removeTrailingComment(line string) string {
-	parts := strings.Split(line, "#")
+	parts := strings.SplitN(line, "#", 2)
-	switch len(parts) {
+	if len(parts) < 2 {
-	case 1:
 		// there aren't any comments
+
 		return line
-	default:
-		// any number of "#" means we only want the first part, assuming this
-		// isn't prefixed with "#" (up to the caller)
-		return parts[0]
 	}
+
+	return parts[0]
+}
+
+// removeEnvironmentMarkers removes any instances of environment markers (delimited by ';') from the line.
+// For more information, see https://www.python.org/dev/peps/pep-0508/#environment-markers.
+func removeEnvironmentMarkers(line string) string {
+	parts := strings.SplitN(line, ";", 2)
+	if len(parts) < 2 {
+		// there aren't any environment markers
+
+		return line
+	}
+
+	return parts[0]
 }

syft/pkg/cataloger/python/parse_requirements_test.go

@@ -4,47 +4,33 @@ import (
 	"os"
 	"testing"
 
-	"github.com/stretchr/testify/assert"
+	"github.com/google/go-cmp/cmp"
-
-	"github.com/go-test/deep"
 
 	"github.com/anchore/syft/syft/pkg"
 )
 
-func assertPackagesEqual(t *testing.T, actual []*pkg.Package, expected map[string]pkg.Package) {
-	t.Helper()
-	if len(actual) != len(expected) {
-		for _, a := range actual {
-			t.Log("   ", a)
-		}
-		t.Fatalf("unexpected package count: %d!=%d", len(actual), len(expected))
-	}
-
-	for _, a := range actual {
-		expectedPkg, ok := expected[a.Name]
-		assert.True(t, ok)
-
-		for _, d := range deep.Equal(a, &expectedPkg) {
-			t.Errorf("diff: %+v", d)
-		}
-	}
-}
-
 func TestParseRequirementsTxt(t *testing.T) {
-	expected := map[string]pkg.Package{
+	expected := []*pkg.Package{
-		"foo": {
+		{
-			Name:     "foo",
-			Version:  "1.0.0",
-			Language: pkg.Python,
-			Type:     pkg.PythonPkg,
-		},
-		"flask": {
 			Name:     "flask",
 			Version:  "4.0.0",
 			Language: pkg.Python,
 			Type:     pkg.PythonPkg,
 		},
+		{
+			Name:     "foo",
+			Version:  "1.0.0",
+			Language: pkg.Python,
+			Type:     pkg.PythonPkg,
+		},
+		{
+			Name:     "SomeProject",
+			Version:  "5.4",
+			Language: pkg.Python,
+			Type:     pkg.PythonPkg,
+		},
 	}
+
 	fixture, err := os.Open("test-fixtures/requires/requirements.txt")
 	if err != nil {
 		t.Fatalf("failed to open fixture: %+v", err)
@@ -56,6 +42,7 @@ func TestParseRequirementsTxt(t *testing.T) {
 		t.Fatalf("failed to parse requirements: %+v", err)
 	}
 
-	assertPackagesEqual(t, actual, expected)
+	if diff := cmp.Diff(expected, actual, cmp.AllowUnexported(pkg.Package{})); diff != "" {
-
+		t.Errorf("unexpected result from parsing (-expected +actual)\n%s", diff)
+	}
 }

syft/pkg/cataloger/python/parse_setup_test.go

@@ -4,52 +4,57 @@ import (
 	"os"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+
 	"github.com/anchore/syft/syft/pkg"
 )
 
 func TestParseSetup(t *testing.T) {
-	expected := map[string]pkg.Package{
+	expected := []*pkg.Package{
-		"pathlib3": {
+		{
 			Name:     "pathlib3",
 			Version:  "2.2.0",
 			Language: pkg.Python,
 			Type:     pkg.PythonPkg,
 		},
-		"mypy": {
+		{
 			Name:     "mypy",
 			Version:  "v0.770",
 			Language: pkg.Python,
 			Type:     pkg.PythonPkg,
 		},
-		"mypy1": {
+		{
 			Name:     "mypy1",
 			Version:  "v0.770",
 			Language: pkg.Python,
 			Type:     pkg.PythonPkg,
 		},
-		"mypy2": {
+		{
 			Name:     "mypy2",
 			Version:  "v0.770",
 			Language: pkg.Python,
 			Type:     pkg.PythonPkg,
 		},
-		"mypy3": {
+		{
 			Name:     "mypy3",
 			Version:  "v0.770",
 			Language: pkg.Python,
 			Type:     pkg.PythonPkg,
 		},
 	}
+
 	fixture, err := os.Open("test-fixtures/setup/setup.py")
 	if err != nil {
 		t.Fatalf("failed to open fixture: %+v", err)
 	}
 
+	// TODO: no relationships are under test yet
 	actual, _, err := parseSetup(fixture.Name(), fixture)
 	if err != nil {
 		t.Fatalf("failed to parse requirements: %+v", err)
 	}
 
-	assertPackagesEqual(t, actual, expected)
+	if diff := cmp.Diff(expected, actual, cmp.AllowUnexported(pkg.Package{})); diff != "" {
-
+		t.Errorf("unexpected result from parsing (-expected +actual)\n%s", diff)
+	}
 }

syft/pkg/cataloger/python/test-fixtures/requires/requirements.txt

@@ -5,3 +5,8 @@ sqlalchemy >= 1.0.0
 -e https://github.com/pecan/pecan.git
 -r other-requirements.txt
 --requirements super-secretrequirements.txt
+SomeProject ==5.4 ; python_version < '3.8'
+coverage != 3.5 # Version Exclusion. Anything except version 3.5
+numpyNew; sys_platform == 'win32'
+numpy >= 3.4.1; sys_platform == 'win32'
+Mopidy-Dirble ~= 1.1 # Compatible release. Same as >= 1.1, == 1.*