semver: reject leading zeroes and empty pre-release/build - #9004
Open
sueun-dev wants to merge 4 commits into
Open
semver: reject leading zeroes and empty pre-release/build#9004sueun-dev wants to merge 4 commits into
sueun-dev wants to merge 4 commits into
Conversation
semver.Parse accepted several strings that are not valid SemVer 2.0.0
versions, so semver.is_valid returned true for them and semver.compare
treated them as ordinary versions:
- a leading zero in the major, minor or patch number ("01.2.3",
"1.02.3")
- a leading zero in a numeric pre-release identifier ("1.2.3-01")
- an empty pre-release or build-metadata section ("1.2.3-", "1.2.3+")
The core numbers were parsed straight through strconv.ParseInt, which
accepts leading zeroes, and the pre-release/metadata identifier check was
skipped whenever the section was empty. The spec forbids all of these.
Validate the major/minor/patch numbers against 0|[1-9][0-9]* before
ParseInt, run the identifier check whenever the separator is present
(rejecting an empty section), and reject a leading zero in numeric
pre-release identifiers. Build-metadata identifiers keep allowing leading
zeroes, as the spec permits. Adds the new invalid cases to TestBadInput
and a TestGoodInput to guard the spec-valid edge cases.
Signed-off-by: Sueun Cho <sueun.dev@gmail.com>
charlieegan3
left a comment
charlieegan3
left a comment
Contributor
There was a problem hiding this comment.
Hey, thanks for this, two small comments. Some of the checks are failing too, please take a look when you have a moment.
|
|
||
| version, v.Metadata = cut(version, '+') | ||
| if v.Metadata != "" && !reMetaIdentifier.MatchString(v.Metadata) { | ||
| var found bool |
Contributor
There was a problem hiding this comment.
Suggested change
| var found bool | |
| var foundMetadata bool |
| // parseNumeric parses a major, minor or patch identifier, rejecting the empty | ||
| // string, a sign or a leading zero (all forbidden by SemVer 2.0.0) before | ||
| // converting to int64. | ||
| func parseNumeric(s string) (int64, error) { |
Contributor
There was a problem hiding this comment.
Perhaps not using a regex might be faster here
func parseNumeric(s string) (int64, error) {
if s == "" || s[0] == '+' || s[0] == '-' || (len(s) > 1 && s[0] == '0') {
return 0, fmt.Errorf("%q is not a valid numeric identifier", s)
}
return strconv.ParseInt(s, 10, 64)
}
charlieegan3
changed the title
Signed-off-by: Sueun Cho <sueun.dev@gmail.com>
Signed-off-by: Sueun Cho <sueun.dev@gmail.com>
…eric-ids Signed-off-by: Sueun Cho <sueun.dev@gmail.com> # Conflicts: # internal/semver/semver.go
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
semver.Parse(behind thesemver.is_validandsemver.comparebuilt-ins) accepts several strings that are not valid SemVer 2.0.0 versions:The major/minor/patch numbers are parsed straight through
strconv.ParseInt, which happily accepts a leading zero, and the pre-release/metadata identifier check is skipped whenever the section is empty (if v.PreRelease != ""). The spec forbids leading zeroes in the numeric identifiers and requires a non-empty pre-release/build section after the-/+.semver.is_validis documented as "Validates that the input is a valid SemVer string", so returningtruehere is wrong, andsemver.comparesilently treats a malformed version as an ordinary one instead of erroring the way it already does for e.g.1.2.Fix
0|[1-9][0-9]*beforeParseInt.-/+separator is present (an empty section is now rejected), usingstrings.Cutfor the found flag.reMetaIdentifieralready rejects other spec violations (e.g._), so this brings the numeric-identifier handling in line with that.Test
Added the new invalid strings to
TestBadInput(they parsed cleanly before, error now) and aTestGoodInputcovering the spec-valid edge cases that must keep parsing — a single-zero pre-release (1.2.3-0), an alphanumeric id starting with zero (1.2.3-0a), and build metadata with a leading zero (1.2.3+01).go test ./internal/semver/and./v1/topdown/pass.