From 199dc343550da3b96f9221e399b76ca4132c0e1a Mon Sep 17 00:00:00 2001 From: Phil Dibowitz Date: Sat, 4 Jul 2026 12:33:57 -0700 Subject: [PATCH] Fix version parsing, validate debian package version This patch is based on one originally by Aaron Foster , 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 Signed-off-by: Phil Dibowitz --- deb/import.go | 6 ++ deb/import_test.go | 47 +++++++++++++++ deb/package.go | 5 +- deb/query.go | 7 ++- deb/query_test.go | 40 +++++++++++++ deb/testdata/import/invalid-version.dsc | 7 +++ deb/version.go | 67 +++++++++++++++++++-- deb/version_test.go | 79 ++++++++++++++++++++++--- 8 files changed, 242 insertions(+), 16 deletions(-) create mode 100644 deb/import_test.go create mode 100644 deb/testdata/import/invalid-version.dsc diff --git a/deb/import.go b/deb/import.go index 99d52be82..9e1b603c4 100644 --- a/deb/import.go +++ b/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) diff --git a/deb/import_test.go b/deb/import_test.go new file mode 100644 index 000000000..f069c5776 --- /dev/null +++ b/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", + }) +} diff --git a/deb/package.go b/deb/package.go index 0af2a4b4a..095a6a723 100644 --- a/deb/package.go +++ b/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 diff --git a/deb/query.go b/deb/query.go index ea5fd255c..229fc53e3 100644 --- a/deb/query.go +++ b/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: diff --git a/deb/query_test.go b/deb/query_test.go index fccd0cf83..6d3917b69 100644 --- a/deb/query_test.go +++ b/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) +} diff --git a/deb/testdata/import/invalid-version.dsc b/deb/testdata/import/invalid-version.dsc new file mode 100644 index 000000000..578a9671c --- /dev/null +++ b/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 +Standards-Version: 3.9.3 diff --git a/deb/version.go b/deb/version.go index 4b9150389..9cd743642 100644 --- a/deb/version.go +++ b/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,15 +43,24 @@ 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 @@ -45,6 +68,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 diff --git a/deb/version_test.go b/deb/version_test.go index 5b0692313..1a52f28b7 100644 --- a/deb/version_test.go +++ b/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) {