Fix version parsing, validate debian package version

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/package.go

@@ -412,12 +412,13 @@ func versionSatisfiesDependency(version string, dep Dependency) bool {
 		return r == 0
 	case VersionLess:
 		return r < 0
+	// 2 is returned when a package with an invalid version is detected; setting boundary for VersionGreater and GreaterOrEqual cases
 	case VersionGreater:
-		return r > 0
+		return r > 0 && r < 2
 	case VersionLessOrEqual:
 		return r <= 0
 	case VersionGreaterOrEqual:
-		return r >= 0
+		return r >= 0 && r < 2
 	case VersionPatternMatch:
 		matched, err := filepath.Match(dep.Version, version)
 		return err == nil && matched

deb/query.go

@@ -169,10 +169,13 @@ func (q *FieldQuery) Matches(pkg PackageLike) bool {
 		return field != ""
 	case VersionEqual:
 		return CompareVersions(field, q.Value) == 0
+	// 2 is returned when a package with an invalid version is detected; setting boundary for VersionGreater and VersionGreaterOrEqual cases
 	case VersionGreater:
-		return CompareVersions(field, q.Value) > 0
+		result := CompareVersions(field, q.Value)
+		return result > 0 && result < 2
 	case VersionGreaterOrEqual:
-		return CompareVersions(field, q.Value) >= 0
+		result := CompareVersions(field, q.Value)
+		return result >= 0 && result < 2
 	case VersionLess:
 		return CompareVersions(field, q.Value) < 0
 	case VersionLessOrEqual:

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/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

@@ -8,12 +8,26 @@ import (
 	"unicode"
 )
 
+var (
+	upstreamVersionRegex = regexp.MustCompile(`^[0-9][A-Za-z0-9.+~\-]*$`)
+	debianRevisionRegex  = regexp.MustCompile(`^[A-Za-z0-9.+~]*$`)
+)
+
 // 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)
+	e1, u1, d1, err := parseVersion(ver1)
+	// if an error is caught during parse, return 2 to signal
+	// an invalid version and handle as needed
+	if err != nil {
+		return 2
+	}
+
+	e2, u2, d2, err := parseVersion(ver2)
+	if err != nil {
+		return 2
+	}
 
 	r := compareVersionPart(e1, e2)
 	if r != 0 {
@@ -29,22 +43,67 @@ func CompareVersions(ver1, ver2 string) int {
 }
 
 // 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
 
 	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) {
@@ -100,7 +154,16 @@ func (s *VersionSuite) TestCompareVersions(c *C) {
 	c.Check(CompareVersions("1.0-133-avc", "1.0"), Equals, 1)
 
 	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)
+	// 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`).
+	c.Check(CompareVersions("4.3.5a", "4.3.5-rc3-1"), Equals, -1)
+
+	// version validation happens independent of CompareVersions, so only testing the
+	// edge case where a package is missing a Debian version during parseVersion
+	c.Check(CompareVersions("1:abc~1.2.3-", "1:1.2.3~abc-good"), Equals, 2)
+	// same edge case, but with the unparsable version as the second argument
+	c.Check(CompareVersions("1:1.2.3~abc-good", "1:abc~1.2.3-"), Equals, 2)
 }
 
 func (s *VersionSuite) TestParseDependency(c *C) {