diff --git a/AUTHORS b/AUTHORS index 0dd5f481..d6218157 100644 --- a/AUTHORS +++ b/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) diff --git a/deb/import.go b/deb/import.go index 99d52be8..9e1b603c 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 00000000..f069c577 --- /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/list.go b/deb/list.go index 19e37593..7395734b 100644 --- a/deb/list.go +++ b/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 } diff --git a/deb/list_test.go b/deb/list_test.go index 4e4768e3..d63a3451 100644 --- a/deb/list_test.go +++ b/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) diff --git a/deb/package.go b/deb/package.go index 0af2a4b4..134e73fd 100644 --- a/deb/package.go +++ b/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 diff --git a/deb/query.go b/deb/query.go index ea5fd255..d089c0bc 100644 --- a/deb/query.go +++ b/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 diff --git a/deb/query_test.go b/deb/query_test.go index fccd0cf8..6d3917b6 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/reflist.go b/deb/reflist.go index 0a5c1831..03a198b2 100644 --- a/deb/reflist.go +++ b/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 } diff --git a/deb/reflist_test.go b/deb/reflist_test.go index 3a7297ea..a9a06984 100644 --- a/deb/reflist_test.go +++ b/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"}) +} diff --git a/deb/testdata/import/invalid-version.dsc b/deb/testdata/import/invalid-version.dsc new file mode 100644 index 00000000..578a9671 --- /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 4b915038..58fd21af 100644 --- a/deb/version.go +++ b/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 diff --git a/deb/version_test.go b/deb/version_test.go index 5b069231..6a366324 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) { @@ -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) {