Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: better handling of whitespace #564

Merged
merged 1 commit into from
Jun 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion classes/comparator.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ class Comparator {
}
}

comp = comp.trim().split(/\s+/).join(' ')
debug('comparator', comp, options)
this.options = options
this.loose = !!options.loose
Expand Down Expand Up @@ -133,7 +134,7 @@ class Comparator {
module.exports = Comparator

const parseOptions = require('../internal/parse-options')
const { re, t } = require('../internal/re')
const { safeRe: re, t } = require('../internal/re')
const cmp = require('../functions/cmp')
const debug = require('../internal/debug')
const SemVer = require('./semver')
Expand Down
64 changes: 37 additions & 27 deletions classes/range.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,26 @@ class Range {
this.loose = !!options.loose
this.includePrerelease = !!options.includePrerelease

// First, split based on boolean or ||
// First reduce all whitespace as much as possible so we do not have to rely
// on potentially slow regexes like \s*. This is then stored and used for
// future error messages as well.
this.raw = range
this.set = range
.trim()
.split(/\s+/)
.join(' ')

// First, split on ||
this.set = this.raw
.split('||')
// map the range to a 2d array of comparators
.map(r => this.parseRange(r.trim()))
.map(r => this.parseRange(r))
// throw out any comparator lists that are empty
// this generally means that it was not a valid range, which is allowed
// in loose mode, but will still throw if the WHOLE range is invalid.
.filter(c => c.length)

if (!this.set.length) {
throw new TypeError(`Invalid SemVer Range: ${range}`)
throw new TypeError(`Invalid SemVer Range: ${this.raw}`)
}

// if we have any that are not the null set, throw out null sets.
Expand All @@ -64,9 +71,7 @@ class Range {

format () {
this.range = this.set
.map((comps) => {
return comps.join(' ').trim()
})
.map((comps) => comps.join(' ').trim())
.join('||')
.trim()
return this.range
Expand All @@ -77,8 +82,6 @@ class Range {
}

parseRange (range) {
range = range.trim()

// memoize range parsing for performance.
// this is a very hot path, and fully deterministic.
const memoOpts =
Expand All @@ -105,9 +108,6 @@ class Range {
// `^ 1.2.3` => `^1.2.3`
range = range.replace(re[t.CARETTRIM], caretTrimReplace)

// normalize spaces
range = range.split(/\s+/).join(' ')

// At this point, the range is completely trimmed and
// ready to be split into comparators.

Expand Down Expand Up @@ -203,7 +203,7 @@ const Comparator = require('./comparator')
const debug = require('../internal/debug')
const SemVer = require('./semver')
const {
re,
safeRe: re,
t,
comparatorTrimReplace,
tildeTrimReplace,
Expand Down Expand Up @@ -257,10 +257,13 @@ const isX = id => !id || id.toLowerCase() === 'x' || id === '*'
// ~1.2.3, ~>1.2.3 --> >=1.2.3 <1.3.0-0
// ~1.2.0, ~>1.2.0 --> >=1.2.0 <1.3.0-0
// ~0.0.1 --> >=0.0.1 <0.1.0-0
const replaceTildes = (comp, options) =>
comp.trim().split(/\s+/).map((c) => {
return replaceTilde(c, options)
}).join(' ')
const replaceTildes = (comp, options) => {
return comp
.trim()
.split(/\s+/)
.map((c) => replaceTilde(c, options))
.join(' ')
}

const replaceTilde = (comp, options) => {
const r = options.loose ? re[t.TILDELOOSE] : re[t.TILDE]
Expand Down Expand Up @@ -298,10 +301,13 @@ const replaceTilde = (comp, options) => {
// ^1.2.0 --> >=1.2.0 <2.0.0-0
// ^0.0.1 --> >=0.0.1 <0.0.2-0
// ^0.1.0 --> >=0.1.0 <0.2.0-0
const replaceCarets = (comp, options) =>
comp.trim().split(/\s+/).map((c) => {
return replaceCaret(c, options)
}).join(' ')
const replaceCarets = (comp, options) => {
return comp
.trim()
.split(/\s+/)
.map((c) => replaceCaret(c, options))
.join(' ')
}

const replaceCaret = (comp, options) => {
debug('caret', comp, options)
Expand Down Expand Up @@ -358,9 +364,10 @@ const replaceCaret = (comp, options) => {

const replaceXRanges = (comp, options) => {
debug('replaceXRanges', comp, options)
return comp.split(/\s+/).map((c) => {
return replaceXRange(c, options)
}).join(' ')
return comp
.split(/\s+/)
.map((c) => replaceXRange(c, options))
.join(' ')
}

const replaceXRange = (comp, options) => {
Expand Down Expand Up @@ -443,12 +450,15 @@ const replaceXRange = (comp, options) => {
const replaceStars = (comp, options) => {
debug('replaceStars', comp, options)
// Looseness is ignored here. star is always as loose as it gets!
return comp.trim().replace(re[t.STAR], '')
return comp
.trim()
.replace(re[t.STAR], '')
}

const replaceGTE0 = (comp, options) => {
debug('replaceGTE0', comp, options)
return comp.trim()
return comp
.trim()
.replace(re[options.includePrerelease ? t.GTE0PRE : t.GTE0], '')
}

Expand Down Expand Up @@ -486,7 +496,7 @@ const hyphenReplace = incPr => ($0,
to = `<=${to}`
}

return (`${from} ${to}`).trim()
return `${from} ${to}`.trim()
}

const testSet = (set, version, options) => {
Expand Down
2 changes: 1 addition & 1 deletion classes/semver.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const debug = require('../internal/debug')
const { MAX_LENGTH, MAX_SAFE_INTEGER } = require('../internal/constants')
const { re, t } = require('../internal/re')
const { safeRe: re, t } = require('../internal/re')

const parseOptions = require('../internal/parse-options')
const { compareIdentifiers } = require('../internal/identifiers')
Expand Down
2 changes: 1 addition & 1 deletion functions/coerce.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const SemVer = require('../classes/semver')
const parse = require('./parse')
const { re, t } = require('../internal/re')
const { safeRe: re, t } = require('../internal/re')

const coerce = (version, options) => {
if (version instanceof SemVer) {
Expand Down
11 changes: 11 additions & 0 deletions internal/re.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,27 @@ exports = module.exports = {}

// The actual regexps go on exports.re
const re = exports.re = []
const safeRe = exports.safeRe = []
const src = exports.src = []
const t = exports.t = {}
let R = 0

const createToken = (name, value, isGlobal) => {
// Replace all greedy whitespace to prevent regex dos issues. These regex are
// used internally via the safeRe object since all inputs in this library get
// normalized first to trim and collapse all extra whitespace. The original
// regexes are exported for userland consumption and lower level usage. A
// future breaking change could export the safer regex only with a note that
// all input should have extra whitespace removed.
const safe = value
.split('\\s*').join('\\s{0,1}')
.split('\\s+').join('\\s')
const index = R++
debug(name, index, value)
t[name] = index
src[index] = value
re[index] = new RegExp(value, isGlobal ? 'g' : undefined)
safeRe[index] = new RegExp(safe, isGlobal ? 'g' : undefined)
}

// The following Regular Expressions can be used for tokenizing,
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
"range.bnf"
],
"tap": {
"check-coverage": true,
"timeout": 30,
"coverage-map": "map.js",
"nyc-arg": [
"--exclude",
Expand Down
39 changes: 39 additions & 0 deletions test/integration/whitespace.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
const { test } = require('tap')
const Range = require('../../classes/range')
const SemVer = require('../../classes/semver')
const Comparator = require('../../classes/comparator')
const validRange = require('../../ranges/valid')
const minVersion = require('../../ranges/min-version')
const minSatisfying = require('../../ranges/min-satisfying')
const maxSatisfying = require('../../ranges/max-satisfying')

const s = (n = 500000) => ' '.repeat(n)

test('regex dos via range whitespace', (t) => {
// a range with this much whitespace would take a few minutes to process if
// any redos susceptible regexes were used. there is a global tap timeout per
// file set in the package.json that will error if this test takes too long.
const r = `1.2.3 ${s()} <1.3.0`

t.equal(new Range(r).range, '1.2.3 <1.3.0')
t.equal(validRange(r), '1.2.3 <1.3.0')
t.equal(minVersion(r).version, '1.2.3')
t.equal(minSatisfying(['1.2.3'], r), '1.2.3')
t.equal(maxSatisfying(['1.2.3'], r), '1.2.3')

t.end()
})

test('semver version', (t) => {
const v = `${s(125)}1.2.3${s(125)}`
const tooLong = `${s()}1.2.3${s()}`
t.equal(new SemVer(v).version, '1.2.3')
t.throws(() => new SemVer(tooLong))
t.end()
})

test('comparator', (t) => {
const c = `${s()}<${s()}1.2.3${s()}`
t.equal(new Comparator(c).value, '<1.2.3')
t.end()
})
8 changes: 7 additions & 1 deletion test/internal/re.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const { test } = require('tap')
const { src, re } = require('../../internal/re')
const { src, re, safeRe } = require('../../internal/re')
const semver = require('../../')

test('has a list of src, re, and tokens', (t) => {
Expand All @@ -13,5 +13,11 @@ test('has a list of src, re, and tokens', (t) => {
for (const i in semver.tokens) {
t.match(semver.tokens[i], Number, 'tokens are numbers')
}

safeRe.forEach(r => {
t.notMatch(r.source, '\\s+', 'safe regex do not contain greedy whitespace')
t.notMatch(r.source, '\\s*', 'safe regex do not contain greedy whitespace')
})

t.end()
})
77 changes: 37 additions & 40 deletions test/map.js
Original file line number Diff line number Diff line change
@@ -1,49 +1,46 @@
const t = require('tap')
const { resolve, join, relative, extname, dirname, basename } = require('path')
const { statSync, readdirSync } = require('fs')
const map = require('../map.js')
const pkg = require('../package.json')

// ensure that the coverage map maps all coverage
const ignore = [
'.git',
'.github',
'.commitlintrc.js',
'.eslintrc.js',
'.eslintrc.local.js',
'node_modules',
'coverage',
'tap-snapshots',
'test',
'fixtures',
]
const ROOT = resolve(__dirname, '..')
const TEST = join(ROOT, 'test')
const IGNORE_DIRS = ['fixtures', 'integration']

const { statSync, readdirSync } = require('fs')
const find = (folder, set = [], root = true) => {
const ent = readdirSync(folder)
set.push(...ent.filter(f => !ignore.includes(f) && /\.m?js$/.test(f)).map(f => folder + '/' + f))
for (const e of ent.filter(f => !ignore.includes(f) && !/\.m?js$/.test(f))) {
if (statSync(folder + '/' + e).isDirectory()) {
find(folder + '/' + e, set, false)
const getFile = (f) => {
try {
if (statSync(f).isFile()) {
return extname(f) === '.js' ? [f] : []
}
} catch {
return []
}
if (!root) {
return
}
return set.map(f => f.slice(folder.length + 1)
.replace(/\\/g, '/'))
.sort((a, b) => a.localeCompare(b))
}

const { resolve } = require('path')
const root = resolve(__dirname, '..')
const walk = (item, res = []) => getFile(item) || readdirSync(item)
.map(f => join(item, f))
.reduce((acc, f) => acc.concat(statSync(f).isDirectory() ? walk(f, res) : getFile(f)), [])
.filter(Boolean)

const sut = find(root)
const tests = find(root + '/test')
t.strictSame(sut, tests, 'test files should match system files')
const map = require('../map.js')
const walkAll = (items, relativeTo) => items
.reduce((acc, f) => acc.concat(walk(join(ROOT, f))), [])
.map((f) => relative(relativeTo, f))
.sort()

for (const testFile of tests) {
t.test(testFile, t => {
t.plan(1)
// cast to an array, since map() can return a string or array
const systemFiles = [].concat(map(testFile))
t.ok(systemFiles.some(sys => sut.includes(sys)), 'test covers a file')
})
}
t.test('tests match system', t => {
const sut = walkAll([pkg.tap['coverage-map'], ...pkg.files], ROOT)
const tests = walkAll([basename(TEST)], TEST)
.filter(f => !IGNORE_DIRS.includes(dirname(f)))

t.strictSame(sut, tests, 'test files should match system files')

for (const f of tests) {
t.test(f, t => {
t.plan(1)
t.ok(sut.includes(map(f)), 'test covers a file')
})
}

t.end()
})