Skip to content

fix: subset false-positive with a prerelease eq and a differing bound - #889

Open
spokodev wants to merge 1 commit into
npm:mainfrom
spokodev:fix/subset-prerelease-eq-false-positive
Open

fix: subset false-positive with a prerelease eq and a differing bound#889
spokodev wants to merge 1 commit into
npm:mainfrom
spokodev:fix/subset-prerelease-eq-false-positive

Conversation

@spokodev

Copy link
Copy Markdown

What

subset(sub, dom) returns true (claims sub ⊆ dom) when it is actually false. It triggers when sub combines an exact prerelease comparator (=X.Y.Z-pre) with a >/>=/</<= bound whose version has a different [major, minor, patch] tuple — a subset false-positive (the dangerous direction):

semver.subset('=1.1.2-alpha <3.1.0', '<1.0.0') // true, must be false

// proof: a version in sub is not in dom, so sub can't be a subset of dom
semver.satisfies('1.1.2-alpha', '=1.1.2-alpha <3.1.0') // true  (in sub)
semver.satisfies('1.1.2-alpha', '<1.0.0')              // false (not in dom)

semver.subset('<3.1.0-0 1.1.2-alpha', '~2.0')  // true, must be false

Adding the <3.1.0 bound (which 1.1.2-alpha satisfies) can only shrink sub, so it should never turn a non-subset into a subset — a monotonicity violation.

Root cause

In simpleSubset, the eqSet-vs-bound checks used satisfies(eq, String(gt), options). satisfies builds a full Range and re-applies node-semver's prerelease-exclusion gating, so a prerelease eq (1.1.2-alpha) is judged not to satisfy a plain bound of a different tuple (<3.1.0). The code then treats the eqSet as inconsistent with sub and returns null (a null set), which subset reports as a subset of everything. In reality the eq version does satisfy sub, because its own =eq comparator admits its prerelease.

This is the same "re-applying full-range prerelease gating on an isolated comparator" issue that PR #867 fixed on the dom side (subset.js:177, :195), but it was left in place on the sub (eqSet) side (:127, :131).

Fix

Test the eq version against the raw bound comparator (gt.test(eq) / lt.test(eq)), mirroring PR #867.

Tests

Added two subset cases (=1.1.2-alpha <3.1.0 ⊄ <1.0.0, <3.1.0-0 1.1.2-alpha ⊄ ~2.0). Both fail on main and pass with the fix; issue #757's case (^10.2.0-beta.2 ⊂ ^10.2.0-beta.1) stays correct and the ranges tests pass.

`subset(sub, dom)` returned `true` when `sub` combined an exact prerelease
comparator (`=X.Y.Z-pre`) with a `>`/`>=`/`<`/`<=` bound of a different
`[major,minor,patch]` tuple, even though `sub` contains a version outside
`dom`:

  subset('=1.1.2-alpha <3.1.0', '<1.0.0')      // true, must be false
  satisfies('1.1.2-alpha', '=1.1.2-alpha <3.1.0') // true  (in sub)
  satisfies('1.1.2-alpha', '<1.0.0')              // false (not in dom)

In `simpleSubset`, the eqSet-vs-bound checks used
`satisfies(eq, String(gt), options)`, which rebuilds a full Range and
re-applies node-semver's prerelease-exclusion gating, so a prerelease `eq`
is judged not to satisfy a plain bound of another tuple. The code then
treats the eqSet as inconsistent and returns `null` (null set), which
`subset` reports as a subset of everything.

Test the eq version against the raw bound comparator instead
(`gt.test(eq)` / `lt.test(eq)`) — the same fix PR npm#867 applied to the
dom-side checks, which this left in place on the eqSet side.
@spokodev
spokodev requested a review from a team as a code owner July 23, 2026 13:05
@mrvonkalus

Copy link
Copy Markdown

Independent corroboration of this bug, plus a regression test that covers the class rather than specific inputs — offered in case it's useful for landing this.

I hit this from the other direction, by differential fuzzing subset() against brute-force ground truth over a finite version universe, with no knowledge that this PR existed. Same root cause you identified: satisfies(eq, String(lt), options) rebuilds a Range and re-applies prerelease gating, so the =X.Y.Z-pre comparator that should vouch for its own prerelease is excluded from the very test that needs it. The set is then misjudged as the null set, and "null set is a subset of everything" yields true for arbitrary dom:

semver.subset('1.0.0-alpha.0 <2.0.0', '<0.0.1')  // true

semver.satisfies('1.0.0-alpha.0', '1.0.0-alpha.0 <2.0.0')  // true  (in sub)
semver.satisfies('1.0.0-alpha.0', '<0.0.1')                // false (not in dom)

Why a table row may not be enough here: the existing suite is 100% green both with and without your fix. I ran npx tap --no-coverage on clean 6e05b76 and on 6e05b76 + your patch — zero not ok lines either way, while the defect is demonstrably present in the unpatched tree. The suite structurally cannot see this class of bug, which is presumably how it survived to 7.8.5.

So rather than more rows, this asserts the defining property over generated ranges:

subset(a, b) === true ⟹ no version satisfies a but not b

Only that direction is checked — subset() quantifies over all versions while the test walks a finite universe, so a false result can't be refuted this way, but a true result can, and the false-positive is the dangerous direction. The generator is a seeded xorshift32, so failures reproduce exactly and CI can't flake.

Against unpatched 7.8.5:

# subset("2.0.1-alpha.0 <=2.2.2", "^1.3.x") returned true,
#   but 2.0.1-alpha.0 satisfies the sub-range and not the super-range
# subset("2.1.1-alpha.0 <=2.1.3", ">2.1.3") returned true,
#   but 2.1.1-alpha.0 satisfies the sub-range and not the super-range
not ok 1 - no unsound subset() results
  found: 4
  wanted: 0

With your fix applied: ok, ~260ms. Full suite 1..51, 0 failures, eslint clean.

Two practical notes if you want it as-is: it has to live inside test/ranges/subset.js rather than its own file (test/map.js enforces 1:1 source-to-test correspondence), and t.plan(cases.length + 1) becomes + 2.

Patch: https://github.com/mrvonkalus/glasshouse/blob/main/patches/semver-subset-soundness-test.patch — happy to open it against your branch, or ignore this entirely if you'd rather keep the PR minimal. Either way the fix looks right to me.

Disclosure: the fuzzing, analysis and this test were produced by an AI agent (Claude) working under my direction; harness and full results are at https://github.com/mrvonkalus/glasshouse and reproduce with npm run verify. The bug is yours — you found it eleven days before this did, and this only rediscovered it independently.

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.

2 participants