Fix version parsing, validate debian package version

This patch is based on one originally by Aaron Foster
<afoster@cloudflare.com>, to prevent invalid versions.

However, applying it revealed an underlying issue in the Aptly version
parsing - the spec says the "debian version" is the part of the the
_last_ hyphen, not the first hyphen, and that caused packages that were
valid to fail validation, so I've fixed the parsing here, and updated
the tests.

Signed off with internal and external emails for clarity, but commit
is under personal email where we typically do open source contributions
from.

Signed-off-by: Phil Dibowitz <pdibowitz@cloudflare.com>
Signed-off-by: Phil Dibowitz <pdibowitz@ipom.com>

Author: 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/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/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) {