From 641c44f44956adfc9be02e2596b49d44ba691c7a Mon Sep 17 00:00:00 2001 From: Dan Luhring Date: Thu, 17 Feb 2022 10:00:16 -0500 Subject: [PATCH] Fix panic in requirements.txt parsing (#834) * Stable sort for pipfile.lock parsing Signed-off-by: Dan Luhring * Adjust python parsing tests to use go-cmp Signed-off-by: Dan Luhring * Add failing cases for requirements.txt parsing Signed-off-by: Dan Luhring * Fix failing cases for requirements.txt parsing Signed-off-by: Dan Luhring * Refactor parseRequirementsTxt Signed-off-by: Dan Luhring * Fix static-analysis failure Signed-off-by: Dan Luhring * Fix comment Signed-off-by: Dan Luhring --- .../cataloger/python/parse_pipfile_lock.go | 6 ++ .../python/parse_pipfile_lock_test.go | 18 ++-- .../cataloger/python/parse_requirements.go | 88 +++++++++++-------- .../python/parse_requirements_test.go | 51 ++++------- syft/pkg/cataloger/python/parse_setup_test.go | 21 +++-- .../test-fixtures/requires/requirements.txt | 7 +- 6 files changed, 108 insertions(+), 83 deletions(-) diff --git a/syft/pkg/cataloger/python/parse_pipfile_lock.go b/syft/pkg/cataloger/python/parse_pipfile_lock.go index 7ea963f47..e1790ce1b 100644 --- a/syft/pkg/cataloger/python/parse_pipfile_lock.go +++ b/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 } diff --git a/syft/pkg/cataloger/python/parse_pipfile_lock_test.go b/syft/pkg/cataloger/python/parse_pipfile_lock_test.go index 2e4fedc9d..d4f3607eb 100644 --- a/syft/pkg/cataloger/python/parse_pipfile_lock_test.go +++ b/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{ - "aio-pika": { + expected := []*pkg.Package{ + { 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) + } } diff --git a/syft/pkg/cataloger/python/parse_requirements.go b/syft/pkg/cataloger/python/parse_requirements.go index e18810505..62443bcfa 100644 --- a/syft/pkg/cataloger/python/parse_requirements.go +++ b/syft/pkg/cataloger/python/parse_requirements.go @@ -22,37 +22,34 @@ 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") - - switch { - case strings.HasPrefix(line, "#"): - // commented out line, skip - continue - case 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 - // XXX only needed if we want to log this, otherwise the next case catches it - continue - case len(strings.Split(line, "==")) == 2: - // remove comments if present - uncommented := removeTrailingComment(line) - // parse a new requirement - parts := strings.Split(uncommented, "==") - name := strings.TrimSpace(parts[0]) - version := strings.TrimSpace(parts[1]) - packages = append(packages, &pkg.Package{ - Name: name, - Version: version, - Language: pkg.Python, - Type: pkg.PythonPkg, - }) - default: + if line == "" { + // nothing to parse on this line continue } + + if strings.HasPrefix(line, "-e") { + // editable packages aren't parsed (yet) + continue + } + + if !strings.Contains(line, "==") { + // a package without a version, or a range (unpinned) which does not tell us + // exactly what will be installed. + continue + } + + // parse a new requirement + parts := strings.Split(line, "==") + name := strings.TrimSpace(parts[0]) + version := strings.TrimSpace(parts[1]) + packages = append(packages, &pkg.Package{ + Name: name, + Version: version, + Language: pkg.Python, + Type: pkg.PythonPkg, + }) } 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, "#") - switch len(parts) { - case 1: + parts := strings.SplitN(line, "#", 2) + if len(parts) < 2 { // 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] } diff --git a/syft/pkg/cataloger/python/parse_requirements_test.go b/syft/pkg/cataloger/python/parse_requirements_test.go index a79ff6393..d57d2e36a 100644 --- a/syft/pkg/cataloger/python/parse_requirements_test.go +++ b/syft/pkg/cataloger/python/parse_requirements_test.go @@ -4,47 +4,33 @@ import ( "os" "testing" - "github.com/stretchr/testify/assert" - - "github.com/go-test/deep" + "github.com/google/go-cmp/cmp" "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{ - "foo": { - Name: "foo", - Version: "1.0.0", - Language: pkg.Python, - Type: pkg.PythonPkg, - }, - "flask": { + expected := []*pkg.Package{ + { 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) + } } diff --git a/syft/pkg/cataloger/python/parse_setup_test.go b/syft/pkg/cataloger/python/parse_setup_test.go index c8106157e..41fc19156 100644 --- a/syft/pkg/cataloger/python/parse_setup_test.go +++ b/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{ - "pathlib3": { + expected := []*pkg.Package{ + { 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) + } } diff --git a/syft/pkg/cataloger/python/test-fixtures/requires/requirements.txt b/syft/pkg/cataloger/python/test-fixtures/requires/requirements.txt index 8b115d3af..d66036398 100644 --- a/syft/pkg/cataloger/python/test-fixtures/requires/requirements.txt +++ b/syft/pkg/cataloger/python/test-fixtures/requires/requirements.txt @@ -4,4 +4,9 @@ sqlalchemy >= 1.0.0 foo == 1.0.0 # a comment that needs to be ignored -e https://github.com/pecan/pecan.git -r other-requirements.txt ---requirements super-secretrequirements.txt \ No newline at end of file +--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.*