Repository navigation
fix: loose mode no longer drops input that is not range syntax - #930
Open
Kumar44developer wants to merge 1 commit into
Open
Kumar44developer wants to merge 1 commit into
Kumar44developer wants to merge 1 commit into
Conversation
In loose mode `parseRange` filtered out every token that did not match
`COMPARATORLOOSE` and kept whatever was left. A range containing text
that is not range syntax therefore parsed successfully as something
weaker than what was written:
new Range('* (4.1.x 4.1.2)', { loose: true }).format() // ''
validRange('* (4.1.x 4.1.2)', { loose: true }) // '*'
new Range('>=1.0.0 (foo)', { loose: true }).format() // '>=1.0.0'
validRange('* (bogus)', { loose: true }) // '*'
The first is the dangerous one: something written to pin 4.1.2 ends up
as `*`, which matches every version. `validRange` reporting `'*'` means
a caller that validates input before storing it is told the string is
fine and then gets a different range than the one it handed in.
`(1.2.3)` already threw, but only because nothing survived the filter
and the empty set was rejected. One valid comparator anywhere in the
range masked the rest, which is why this looked inconsistent.
Rejecting everything unparseable is the obvious fix and it is wrong. I
measured it: loose mode relies on the drop for the leftovers of its own
normalization, and three test files fail, among them `== 1`,
`== 1.0.0 || foo`, `1.2.3 || 1.2.3= x` and `1.2.3-dev= 1 || 2.0.0`, all
of which are pinned as parsing today.
So the rule is narrower: a token is dropped as before unless it carries a
character that appears in no comparator and no version, in which case the
range is invalid. A stray `==` or a bare `foo` is sloppy range syntax;
`(foo)` is not range syntax. The error is the `Invalid SemVer Range` the
constructor already raises, so nothing new appears in the API.
The comma is deliberately excluded from that character class.
`1.2.3, <2.0.0` parses as `<2.0.0` today -- the same shape of problem,
the lower bound silently vanishing -- but it is pinned by a test in
test/ranges/valid.js, so changing it is a separate decision for the
maintainers rather than something to slip in here. It is called out in
the pull request and in a comment next to the character class.
Strict mode is untouched: it already threw `Invalid comparator` for all
of these.
Tests: six rows in the range-parse fixture, which runs each input in all
four option combinations, so the parenthesised forms are now asserted
invalid in loose and strict alike. Two focused tests either side of the
line -- one that the bracketed forms are rejected and `validRange`
returns null, one that loose mode still parses its own leftovers,
including the comma case, so a future widening of this check gets caught.
The fixture's hardcoded row counts move from 348/335 to 354/341.
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
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.
Closes #387.
The problem
In loose mode
parseRangefiltered out every token that did not matchCOMPARATORLOOSEand kept whatever was left. So a range containing text that is not range syntax parsed successfully, as something weaker than what was written:The first one is the dangerous case. Something written to pin 4.1.2 comes out as
*, matching every version. AndvalidRangeanswering'*'means a caller that validates input before storing it is told the string is fine, then gets a different range than the one it handed in.(1.2.3)already threw, but only because nothing survived the filter and the empty set was rejected afterwards. A single valid comparator anywhere in the range masked everything else, which is why the behaviour looked arbitrary.Why the obvious fix is wrong
Rejecting anything unparseable is the first thing to try, and I measured it before writing this: three test files fail, because loose mode relies on that drop for the leftovers of its own normalization. These are all pinned as parsing today:
So the drop is load-bearing and cannot simply go.
The rule this uses instead
A token is dropped exactly as before, unless it carries a character that appears in no comparator and no version — then the range is invalid. A stray
==or a barefoois sloppy range syntax;(foo)is not range syntax at all.The error is the
Invalid SemVer Rangethe constructor already raises, so no new error type appears in the API. Strict mode is untouched — it already threwInvalid comparatorfor every one of these.One case I found and deliberately did not change
The comma is excluded from that character class on purpose:
That is the same shape of problem — the
1.2.3lower bound silently vanishes and the range gets weaker. But it is pinned by an explicit test intest/ranges/valid.js, which tells me it is either intentional or long-grandfathered, and quietly reversing it under a different issue number would be the wrong way to find out. It keeps working, and there is a comment next to the character class saying why. If you want it rejected too I'll do it here or in a follow-up — your call.Tests
Six rows in the
range-parsefixture, which runs every input through all four option combinations, so the parenthesised forms are now asserted invalid in loose and strict alike. Then two focused tests, one either side of the line:loose mode does not drop input that is not range syntax— the bracketed forms throw andvalidRangereturnsnull.loose mode still tolerates its own leftovers—== 1,foo,1.2.3= x, the comma case,v1.2.3,=1.2.3all still parse. This is the one that would catch a future widening of the check.The fixture's hardcoded row counts move from 348/335 to 354/341, which is the meta-test in
test/classes/range.jsnoticing the new rows.Verified with the fix reverted: the new fixture rows and both focused tests fail. With it:
npx tapgreen across all 53 test files,npx eslint .clean.