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) {