Merge pull request #1601 from jaymzh/debian-version

Fix version parsing, validate debian package version

AUTHORS

@@ -85,3 +85,4 @@ List of contributors, in chronological order:
 * Tom Nguyen (https://github.com/lecafard)
 * Philip Cramer (https://github.com/PhilipCramer)
 * Kerem Gur (https://github.com/KereMath)
+* Phil Dibowitz (https://github.com/jaymzh)

deb/import.go

@@ -124,6 +124,12 @@ func ImportPackageFiles(list *PackageList, packageFiles []string, forceReplace b
 			continue
 		}
 
+		if !isValidVersion(p.Version) {
+			reporter.Warning("Version number ('%s') for the '%s' package is invalid", p.Version, p.Name)
+			failedFiles = append(failedFiles, file)
+			continue
+		}
+
 		if p.Architecture == "" {
 			reporter.Warning("Empty architecture on %s", file)
 			failedFiles = append(failedFiles, file)

deb/import_test.go

@@ -0,0 +1,47 @@
+package deb
+
+import (
+	"github.com/aptly-dev/aptly/aptly"
+	"github.com/aptly-dev/aptly/database"
+	"github.com/aptly-dev/aptly/database/goleveldb"
+
+	. "gopkg.in/check.v1"
+)
+
+type ImportSuite struct {
+	db                database.Storage
+	packageCollection *PackageCollection
+	reporter          *aptly.RecordingResultReporter
+}
+
+var _ = Suite(&ImportSuite{})
+
+func (s *ImportSuite) SetUpTest(c *C) {
+	s.db, _ = goleveldb.NewOpenDB(c.MkDir())
+	s.packageCollection = NewPackageCollection(s.db)
+	s.reporter = &aptly.RecordingResultReporter{
+		Warnings:     []string{},
+		AddedLines:   []string{},
+		RemovedLines: []string{},
+	}
+}
+
+func (s *ImportSuite) TearDownTest(c *C) {
+	_ = s.db.Close()
+}
+
+func (s *ImportSuite) TestImportPackageFilesRejectsInvalidVersion(c *C) {
+	list := NewPackageList()
+
+	processedFiles, failedFiles, err := ImportPackageFiles(
+		list, []string{"testdata/import/invalid-version.dsc"}, false, &NullVerifier{},
+		nil, s.packageCollection, s.reporter, nil,
+		func(database.ReaderWriter) aptly.ChecksumStorage { return nil })
+
+	c.Assert(err, IsNil)
+	c.Check(processedFiles, HasLen, 0)
+	c.Check(failedFiles, DeepEquals, []string{"testdata/import/invalid-version.dsc"})
+	c.Check(s.reporter.Warnings, DeepEquals, []string{
+		"Version number ('1.2.3-') for the 'aptly-test-invalid-version' package is invalid",
+	})
+}

deb/list.go

@@ -184,8 +184,21 @@ func (l *PackageList) FilterLatest() (*PackageList, error) {
 	err := l.ForEach(func(p *Package) error {
 		key := p.Architecture + "|" + p.Name
 
-		if existing, found := filtered[key]; !found || CompareVersions(p.Version, existing.Version) > 0 {
+		existing, found := filtered[key]
+		if !found {
 			filtered[key] = p
+			return nil
+		}
+
+		cmp, compareErr := CompareVersions(p.Version, existing.Version)
+		switch compareErr {
+		case nil:
+			if cmp > 0 {
+				filtered[key] = p
+			}
+		case RightInvalid:
+			filtered[key] = p
+		case LeftInvalid, BothInvalid:
 		}
 
 		return nil
@@ -427,7 +440,13 @@ func (l *PackageList) Swap(i, j int) {
 
 func (l *PackageList) lessPackages(iPkg, jPkg *Package) bool {
 	if iPkg.Name == jPkg.Name {
-		cmp := CompareVersions(iPkg.Version, jPkg.Version)
+		cmp, err := CompareVersions(iPkg.Version, jPkg.Version)
+		switch err {
+		case LeftInvalid, BothInvalid:
+			return false
+		case RightInvalid:
+			return true
+		}
 		if cmp == 0 {
 			return iPkg.Architecture < jPkg.Architecture
 		}

deb/list_test.go

@@ -531,6 +531,36 @@ func (s *PackageListSuite) TestFilterLatest(c *C) {
 	c.Check(filtered.Has(sharedPkg), Equals, true)
 }
 
+func (s *PackageListSuite) TestFilterLatestInvalidVersions(c *C) {
+	list := NewPackageList()
+	valid := &Package{Name: "pkg", Version: "1.0", Architecture: "amd64"}
+	invalid := &Package{Name: "pkg", Version: "2.0-", Architecture: "amd64"}
+	_ = list.Add(valid)
+	_ = list.Add(invalid)
+
+	filtered, err := list.FilterLatest()
+	c.Assert(err, IsNil)
+	c.Check(filtered.Len(), Equals, 1)
+	c.Check(filtered.Has(valid), Equals, true)
+
+	invalidOnly := NewPackageList()
+	_ = invalidOnly.Add(&Package{Name: "pkg", Version: "1.0-", Architecture: "amd64"})
+	_ = invalidOnly.Add(&Package{Name: "pkg", Version: "2.0-", Architecture: "amd64"})
+	filtered, err = invalidOnly.FilterLatest()
+	c.Assert(err, IsNil)
+	c.Check(filtered.Len(), Equals, 1)
+}
+
+func (s *PackageListSuite) TestLessPackagesInvalidVersions(c *C) {
+	list := NewPackageList()
+	valid := &Package{Name: "pkg", Version: "1.0", Architecture: "amd64"}
+	invalid := &Package{Name: "pkg", Version: "2.0-", Architecture: "amd64"}
+
+	c.Check(list.lessPackages(valid, invalid), Equals, true)
+	c.Check(list.lessPackages(invalid, valid), Equals, false)
+	c.Check(list.lessPackages(invalid, invalid), Equals, false)
+}
+
 func (s *PackageListSuite) TestFilterLatestPreservesDuplicatesFlag(c *C) {
 	list := NewPackageListWithDuplicates(true, 2)
 

deb/package.go

@@ -405,19 +405,19 @@ func (p *Package) MatchesDependency(dep Dependency) bool {
 }
 
 func versionSatisfiesDependency(version string, dep Dependency) bool {
-	r := CompareVersions(version, dep.Version)
+	r, err := CompareVersions(version, dep.Version)
 
 	switch dep.Relation {
 	case VersionEqual:
-		return r == 0
+		return err == nil && r == 0
 	case VersionLess:
-		return r < 0
+		return err == nil && r < 0
 	case VersionGreater:
-		return r > 0
+		return err == nil && r > 0
 	case VersionLessOrEqual:
-		return r <= 0
+		return err == nil && r <= 0
 	case VersionGreaterOrEqual:
-		return r >= 0
+		return err == nil && r >= 0
 	case VersionPatternMatch:
 		matched, err := filepath.Match(dep.Version, version)
 		return err == nil && matched

deb/query.go

@@ -168,15 +168,20 @@ func (q *FieldQuery) Matches(pkg PackageLike) bool {
 	case VersionDontCare:
 		return field != ""
 	case VersionEqual:
-		return CompareVersions(field, q.Value) == 0
+		result, err := CompareVersions(field, q.Value)
+		return err == nil && result == 0
 	case VersionGreater:
-		return CompareVersions(field, q.Value) > 0
+		result, err := CompareVersions(field, q.Value)
+		return err == nil && result > 0
 	case VersionGreaterOrEqual:
-		return CompareVersions(field, q.Value) >= 0
+		result, err := CompareVersions(field, q.Value)
+		return err == nil && result >= 0
 	case VersionLess:
-		return CompareVersions(field, q.Value) < 0
+		result, err := CompareVersions(field, q.Value)
+		return err == nil && result < 0
 	case VersionLessOrEqual:
-		return CompareVersions(field, q.Value) <= 0
+		result, err := CompareVersions(field, q.Value)
+		return err == nil && result <= 0
 	case VersionPatternMatch:
 		matched, err := filepath.Match(q.Value, field)
 		return err == nil && matched

deb/query_test.go

@@ -21,3 +21,43 @@ func (s *QuerySuite) TestVersionCompare(c *C) {
 	c.Check(q.Matches(&p100), Equals, false)
 	c.Check(q.Matches(&p1), Equals, true)
 }
+
+func (s *QuerySuite) TestVersionCompareGreater(c *C) {
+	q := FieldQuery{"Version", VersionGreater, "5.0.0.2", nil}
+
+	p100 := Package{}
+	p100.Version = "5.0.0.100"
+
+	p1 := Package{}
+	p1.Version = "5.0.0.1"
+
+	c.Check(q.Matches(&p100), Equals, true)
+	c.Check(q.Matches(&p1), Equals, false)
+
+	// invalid version on either side of the comparison must not match
+	pInvalid := Package{}
+	pInvalid.Version = "1.2.3-"
+	c.Check(q.Matches(&pInvalid), Equals, false)
+}
+
+func (s *QuerySuite) TestVersionCompareGreaterOrEqual(c *C) {
+	q := FieldQuery{"Version", VersionGreaterOrEqual, "5.0.0.2", nil}
+
+	p100 := Package{}
+	p100.Version = "5.0.0.100"
+
+	pEqual := Package{}
+	pEqual.Version = "5.0.0.2"
+
+	p1 := Package{}
+	p1.Version = "5.0.0.1"
+
+	c.Check(q.Matches(&p100), Equals, true)
+	c.Check(q.Matches(&pEqual), Equals, true)
+	c.Check(q.Matches(&p1), Equals, false)
+
+	// invalid version on either side of the comparison must not match
+	pInvalid := Package{}
+	pInvalid.Version = "1.2.3-"
+	c.Check(q.Matches(&pInvalid), Equals, false)
+}

deb/reflist.go

@@ -365,32 +365,36 @@ func (l *PackageRefList) Merge(r *PackageRefList, overrideMatching, ignoreConfli
 func (l *PackageRefList) FilterLatestRefs() {
 	var (
 		lastArch, lastName, lastVer []byte
-		arch, name, ver             []byte
-		parts                       [][]byte
 	)
 
-	for i := 0; i < len(l.Refs); i++ {
-		parts = bytes.Split(l.Refs[i][1:], []byte(" "))
-		arch, name, ver = parts[0], parts[1], parts[2]
+	filtered := l.Refs[:0]
+	for _, ref := range l.Refs {
+		parts := bytes.Split(ref[1:], []byte(" "))
+		arch, name, ver := parts[0], parts[1], parts[2]
 
 		if bytes.Equal(arch, lastArch) && bytes.Equal(name, lastName) {
-			// Two packages are identical, check version and only one wins
-			vres := CompareVersions(string(ver), string(lastVer))
-
-			// Remove the older refs from the result
-			if vres > 0 {
-				// ver[i] > ver[i-1], remove element i-1
-				l.Refs = append(l.Refs[:i-1], l.Refs[i:]...)
-			} else {
-				// ver[i] < ver[i-1], remove element i
-				l.Refs = append(l.Refs[:i], l.Refs[i+1:]...)
-				arch, name, ver = lastArch, lastName, lastVer
+			// The previous ref is valid, so this also validates the current ref.
+			vres, err := CompareVersions(string(ver), string(lastVer))
+			if err != nil {
+				continue
 			}
 
-			// Compensate for the reduced set
-			i--
+			// Replace the older ref in the result.
+			if vres > 0 {
+				filtered[len(filtered)-1] = ref
+				lastArch, lastName, lastVer = arch, name, ver
+			}
+			continue
 		}
 
+		// Comparing a version with itself validates singleton refs.
+		if _, err := CompareVersions(string(ver), string(ver)); err != nil {
+			continue
+		}
+
+		filtered = append(filtered, ref)
 		lastArch, lastName, lastVer = arch, name, ver
 	}
+
+	l.Refs = filtered
 }

deb/reflist_test.go

@@ -398,3 +398,17 @@ func (s *PackageRefListSuite) TestFilterLatestRefs(c *C) {
 	c.Check(toStrSlice(result), DeepEquals,
 		[]string{"Pi386 dpkg 1.6", "Pi386 lib 1.2"})
 }
+
+func (s *PackageRefListSuite) TestFilterLatestRefsRemovesInvalidVersions(c *C) {
+	result := &PackageRefList{Refs: [][]byte{
+		[]byte("Pi386 app 1.0-"),
+		[]byte("Pi386 dpkg 1.0"),
+		[]byte("Pi386 dpkg 2.0-"),
+		[]byte("Pi386 lib 1.0-"),
+		[]byte("Pi386 lib 2.0-"),
+	}}
+
+	result.FilterLatestRefs()
+
+	c.Check(toStrSlice(result), DeepEquals, []string{"Pi386 dpkg 1.0"})
+}

deb/testdata/import/invalid-version.dsc

@@ -0,0 +1,7 @@
+Format: 1.0
+Source: aptly-test-invalid-version
+Binary: aptly-test-invalid-version
+Architecture: any
+Version: 1.2.3-
+Maintainer: Aptly Test <test@example.com>
+Standards-Version: 3.9.3

deb/version.go

@@ -6,38 +6,86 @@ import (
 	"strconv"
 	"strings"
 	"unicode"
+
+	"github.com/rs/zerolog/log"
+)
+
+var (
+	upstreamVersionRegex = regexp.MustCompile(`^[0-9][A-Za-z0-9.+~\-]*$`)
+	debianRevisionRegex  = regexp.MustCompile(`^[A-Za-z0-9.+~]*$`)
+)
+
+// VersionCompareError identifies an invalid operand passed to CompareVersions.
+type VersionCompareError string
+
+// Error implements the error interface.
+func (e VersionCompareError) Error() string {
+	return string(e)
+}
+
+// CompareVersions errors.
+const (
+	LeftInvalid  VersionCompareError = "left version is invalid"
+	RightInvalid VersionCompareError = "right version is invalid"
+	BothInvalid  VersionCompareError = "both versions are invalid"
 )
 
 // Using documentation from: http://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-Version
 
 // CompareVersions compares two package versions
-func CompareVersions(ver1, ver2 string) int {
-	e1, u1, d1 := parseVersion(ver1)
-	e2, u2, d2 := parseVersion(ver2)
+func CompareVersions(ver1, ver2 string) (int, error) {
+	e1, u1, d1, err1 := parseVersion(ver1)
+	e2, u2, d2, err2 := parseVersion(ver2)
+
+	if err1 != nil {
+		log.Warn().Err(err1).Str("version", ver1).Msg("Invalid package version")
+	}
+	if err2 != nil && (err1 == nil || ver1 != ver2) {
+		log.Warn().Err(err2).Str("version", ver2).Msg("Invalid package version")
+	}
+
+	if err1 != nil && err2 != nil {
+		return 0, BothInvalid
+	}
+	if err1 != nil {
+		return 0, LeftInvalid
+	}
+	if err2 != nil {
+		return 0, RightInvalid
+	}
 
 	r := compareVersionPart(e1, e2)
 	if r != 0 {
-		return r
+		return r, nil
 	}
 
 	r = compareVersionPart(u1, u2)
 	if r != 0 {
-		return r
+		return r, nil
 	}
 
-	return compareVersionPart(d1, d2)
+	return compareVersionPart(d1, d2), nil
 }
 
 // parseVersions breaks down full version to components (possibly empty)
-func parseVersion(ver string) (epoch, upstream, debian string) {
+func parseVersion(ver string) (epoch, upstream, debian string, err error) {
 	i := strings.Index(ver, ":")
 	if i != -1 {
 		epoch, ver = ver[:i], ver[i+1:]
 	}
 
-	i = strings.Index(ver, "-")
+	// Debian policy specifies that the upstream_version and debian_revision
+	// are separated by the LAST hyphen in the string, since upstream_version
+	// itself may legitimately contain hyphens.
+	i = strings.LastIndex(ver, "-")
 	if i != -1 {
 		debian, ver = ver[i+1:], ver[:i]
+		if debian == "" {
+			// if a hyphen is detected in the upstream version
+			// string without a debian revision following it, the
+			// version is invalid
+			return "", "", "", fmt.Errorf("could not parse version: version string ('%s-') includes hyphen without Debian revision", ver)
+		}
 	}
 
 	upstream = ver
@@ -45,6 +93,42 @@ func parseVersion(ver string) (epoch, upstream, debian string) {
 	return
 }
 
+// isValidVersion checks whether package version is compliant with control field spec
+// source: https://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-version
+func isValidVersion(ver string) bool {
+	epoch, upstream, deb, err := parseVersion(ver)
+	if err != nil {
+		return false
+	}
+
+	// validate epoch component
+	if epoch != "" {
+		// uint64 for unexpectedly high epoch values
+		_, err := strconv.ParseUint(epoch, 10, 64)
+		if err != nil {
+			return false
+		}
+	}
+
+	if upstream == "" || !isValidUpstreamVersion(upstream) {
+		return false
+	}
+
+	if deb != "" && !isValidDebianRevision(deb) {
+		return false
+	}
+
+	return true
+}
+
+func isValidUpstreamVersion(upstream string) bool {
+	return upstreamVersionRegex.MatchString(upstream)
+}
+
+func isValidDebianRevision(debian string) bool {
+	return debianRevisionRegex.MatchString(debian)
+}
+
 // compareLexicographic compares in "Debian lexicographic" way, see below compareVersionPart for details
 func compareLexicographic(s1, s2 string) int {
 	i := 0

deb/version_test.go

@@ -10,20 +10,74 @@ type VersionSuite struct {
 var _ = Suite(&VersionSuite{})
 
 func (s *VersionSuite) TestParseVersion(c *C) {
-	e, u, d := parseVersion("1.3.4")
+	e, u, d, err := parseVersion("1.3.4")
 	c.Check([]string{e, u, d}, DeepEquals, []string{"", "1.3.4", ""})
+	c.Check(err, Equals, nil)
 
-	e, u, d = parseVersion("4:1.3:4")
+	e, u, d, err = parseVersion("4:1.3:4")
 	c.Check([]string{e, u, d}, DeepEquals, []string{"4", "1.3:4", ""})
+	c.Check(err, Equals, nil)
 
-	e, u, d = parseVersion("1.3.4-1")
+	e, u, d, err = parseVersion("1.3.4-1")
 	c.Check([]string{e, u, d}, DeepEquals, []string{"", "1.3.4", "1"})
+	c.Check(err, Equals, nil)
 
-	e, u, d = parseVersion("1.3-pre4-1")
-	c.Check([]string{e, u, d}, DeepEquals, []string{"", "1.3", "pre4-1"})
+	// upstream_version and debian_revision are separated by the LAST
+	// hyphen, since upstream_version may itself contain hyphens.
+	e, u, d, err = parseVersion("1.3-pre4-1")
+	c.Check([]string{e, u, d}, DeepEquals, []string{"", "1.3-pre4", "1"})
+	c.Check(err, Equals, nil)
 
-	e, u, d = parseVersion("4:1.3-pre4-1")
-	c.Check([]string{e, u, d}, DeepEquals, []string{"4", "1.3", "pre4-1"})
+	e, u, d, err = parseVersion("4:1.3-pre4-1")
+	c.Check([]string{e, u, d}, DeepEquals, []string{"4", "1.3-pre4", "1"})
+	c.Check(err, Equals, nil)
+
+	e, u, d, err = parseVersion("1:2026.07.07-0325-54-a773b756")
+	c.Check([]string{e, u, d}, DeepEquals, []string{"1", "2026.07.07-0325-54", "a773b756"})
+	c.Check(err, Equals, nil)
+
+	e, u, d, err = parseVersion("1:1.2024-")
+	c.Check([]string{e, u, d}, DeepEquals, []string{"", "", ""})
+	c.Check(err.Error(), Equals, "could not parse version: version string ('1.2024-') includes hyphen without Debian revision")
+}
+
+func (s *VersionSuite) TestIsValidVersion(c *C) {
+	// valid cases
+	valid := isValidVersion("1.2.3-abc")
+	c.Check(valid, Equals, true)
+
+	valid = isValidVersion("1.0.1337~rc2-3")
+	c.Check(valid, Equals, true)
+
+	valid = isValidVersion("1.2.3+fdsfgs")
+	c.Check(valid, Equals, true)
+
+	valid = isValidVersion("1:2.3~4-5six")
+	c.Check(valid, Equals, true)
+
+	// upstream_version containing multiple hyphens (e.g. date/build-number/
+	// git-sha style versioning) is valid as long as it's split on the LAST
+	// hyphen for the debian_revision.
+	valid = isValidVersion("1:2026.07.07-0325-54-a773b756")
+	c.Check(valid, Equals, true)
+
+	// invalid cases
+	valid = isValidVersion("1:1.2.3-")
+	c.Check(valid, Equals, false)
+
+	valid = isValidVersion("42:")
+	c.Check(valid, Equals, false)
+
+	valid = isValidVersion("1:a.1.2.3~-4")
+	c.Check(valid, Equals, false)
+
+	// non-numeric epoch
+	valid = isValidVersion("abc:1.2.3")
+	c.Check(valid, Equals, false)
+
+	// valid upstream_version, but debian_revision contains a disallowed character
+	valid = isValidVersion("1.2.3-abc!")
+	c.Check(valid, Equals, false)
 }
 
 func (s *VersionSuite) TestCompareLexicographic(c *C) {
@@ -86,21 +140,35 @@ func (s *VersionSuite) TestCompareVersionPart(c *C) {
 }
 
 func (s *VersionSuite) TestCompareVersions(c *C) {
-	c.Check(CompareVersions("3:1.0~beta1~svn1245-1", "3:1.0~beta1~svn1245-1"), Equals, 0)
+	check := func(left, right string, expectedCmp int, expectedErr error) {
+		cmp, err := CompareVersions(left, right)
+		c.Check(cmp, Equals, expectedCmp)
+		c.Check(err, Equals, expectedErr)
+	}
 
-	c.Check(CompareVersions("1:1.0~beta1~svn1245-1", "3:1.0~beta1~svn1245-1"), Equals, -1)
-	c.Check(CompareVersions("1:1.0~beta1~svn1245-1", "1.0~beta1~svn1245-1"), Equals, 1)
-	c.Check(CompareVersions("1.0~beta1~svn1245-1", "1.0~beta1~svn1245-2"), Equals, -1)
-	c.Check(CompareVersions("3:1.0~beta1~svn1245-1", "3:1.0~beta1-1"), Equals, -1)
+	check("3:1.0~beta1~svn1245-1", "3:1.0~beta1~svn1245-1", 0, nil)
 
-	c.Check(CompareVersions("1.0~beta1~svn1245", "1.0~beta1"), Equals, -1)
-	c.Check(CompareVersions("1.0~beta1", "1.0"), Equals, -1)
+	check("1:1.0~beta1~svn1245-1", "3:1.0~beta1~svn1245-1", -1, nil)
+	check("1:1.0~beta1~svn1245-1", "1.0~beta1~svn1245-1", 1, nil)
+	check("1.0~beta1~svn1245-1", "1.0~beta1~svn1245-2", -1, nil)
+	check("3:1.0~beta1~svn1245-1", "3:1.0~beta1-1", -1, nil)
 
-	c.Check(CompareVersions("1.0-133-avc", "1.1"), Equals, -1)
-	c.Check(CompareVersions("1.0-133-avc", "1.0"), Equals, 1)
+	check("1.0~beta1~svn1245", "1.0~beta1", -1, nil)
+	check("1.0~beta1", "1.0", -1, nil)
 
-	c.Check(CompareVersions("5.2.0.3", "5.2.0.283"), Equals, -1)
-	c.Check(CompareVersions("4.3.5a", "4.3.5-rc3-1"), Equals, 1)
+	check("1.0-133-avc", "1.1", -1, nil)
+	check("1.0-133-avc", "1.0", 1, nil)
+
+	check("5.2.0.3", "5.2.0.283", -1, nil)
+	// upstream_version/debian_revision split on the LAST hyphen: "4.3.5-rc3-1"
+	// is upstream="4.3.5-rc3", debian="1", so it's actually greater than "4.3.5a"
+	// (confirmed against `dpkg --compare-versions`).
+	check("4.3.5a", "4.3.5-rc3-1", -1, nil)
+
+	// Version validation happens independently, so these exercise parse failures.
+	check("1:abc~1.2.3-", "1:1.2.3~abc-good", 0, LeftInvalid)
+	check("1:1.2.3~abc-good", "1:abc~1.2.3-", 0, RightInvalid)
+	check("1:abc~1.2.3-", "2:def~4.5.6-", 0, BothInvalid)
 }
 
 func (s *VersionSuite) TestParseDependency(c *C) {