Skip to content

fix: reject '|' inside a constraint's version segments - #320

Open
januththedev wants to merge 1 commit into
Masterminds:masterfrom
januththedev:fix/constraint-pipe-charclass
Open

januththedev wants to merge 1 commit into
Masterminds:masterfrom
januththedev:fix/constraint-pipe-charclass

Conversation

@januththedev

Copy link
Copy Markdown

| was accepted inside a constraint's version segments

Description

cvRegex is written with a character class where an alternation was clearly intended:

const cvRegex string = `v?([0-9|x|X|\*]+)(\.[0-9|x|X|\*]+)?(\.[0-9|x|X|\*]+)?` + ...

Inside [...], | is a literal member character, not an alternation operator. The intent was (0-9|x|X|\*). The only unintended member is |, so | is now accepted anywhere a version segment may appear — even though it is this grammar's OR separator and is never legal in a version segment per semver.org.

cvRegex builds constraintRegex, constraintRangeRegex, findConstraintRegex and validConstraintRegex, so this affects the whole constraint grammar.

Two concrete failure modes

(a) Garbage silently accepted, and the input discarded. In parseConstraint, when the major segment is a wildcard the code takes the isX(m[3]) branch and builds ver = "0.0.0" + m[6] — dropping m[3] entirely, so it is never validated:

input current result should be
*.|2 accepted; Check == * for every version an error
*.2|3 accepted; Check == * an error
1.*.2|3 accepted; Check == 1.x an error

A clearly malformed constraint parses successfully and then evaluates as a completely different, far more permissive range.

(b) The library's internal "unreachable" branch becomes reachable. constraints.go:311 and :330 are both commented "The constraintRegex should catch any regex parsing errors. So, we should never get here." and return fmt.Errorf("constraint parser error"). Because cvRegex wrongly accepts |, such strings survive validConstraintRegex, then fail inside NewVersion during parseConstraint — so user input reaches a path documented as impossible, and callers get the opaque internal error instead of the public improper constraint: %q.

Affected: 1|2, 1.2|3, 1.2.3|4, x|2, *|2, >=1.2.3|, >=1.2.3 | 2.0.0 (a single-pipe typo for ||), and 1.2.3 | | 2.0.0.

The change

-const cvRegex string = `v?([0-9|x|X|\*]+)(\.[0-9|x|X|\*]+)?(\.[0-9|x|X|\*]+)?` +
+const cvRegex string = `v?([0-9xX*]+)(\.[0-9xX*]+)?(\.[0-9xX*]+)?` +

Dropping the | is sufficient: x, X and * were already member characters, so removing the pipe makes the class exactly the set the author intended.

Tests

TestNewConstraintRejectsPipeInVersion covers 11 malformed inputs above, asserting each now returns the public improper constraint error rather than being accepted or returning the internal constraint parser error. It also asserts 15 legal wildcard constraints (*, x, X, 1.x, 1.X, 1.2.x, *.2, 1.*, *.*.*, ^1.x, ~1.x, >=1.x, <=2.x, !=1.x) still parse, so the class cannot be over-narrowed.

  • Before: all 11 assertions fail. After: pass.
  • go test ./... → 50 top-level tests, 43 subtests, 0 failures. The baseline on unmodified HEAD was 49 top-level with 0 failures, so the delta is exactly this one test.
  • go vet ./... clean.
  • FuzzNewConstraint 76,403 execs and FuzzNewVersion 931,069 execs pass, with no crashers.

I checked the 22 open PRs and 24 open issues: nothing touches cvRegex or the | character. PR #315 covers a separate comparePrePart overflow that I also found and discarded as in-flight.

@januththedev
januththedev force-pushed the fix/constraint-pipe-charclass branch from 6a89e72 to 2ce10f5 Compare October 3, 2026 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant