fix: better handling of whitespace (#564)

Author: lukekarrys

Diff for: classes/comparator.js

@@ -16,6 +16,7 @@ class Comparator {
       }
     }
 
+    comp = comp.trim().split(/\s+/).join(' ')
     debug('comparator', comp, options)
     this.options = options
     this.loose = !!options.loose
@@ -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')

Diff for: classes/range.js

@@ -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.
@@ -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
@@ -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 =
@@ -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.
 
@@ -203,7 +203,7 @@ const Comparator = require('./comparator')
 const debug = require('../internal/debug')
 const SemVer = require('./semver')
 const {
-  re,
+  safeRe: re,
   t,
   comparatorTrimReplace,
   tildeTrimReplace,
@@ -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]
@@ -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)
@@ -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) => {
@@ -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], '')
 }
 
@@ -486,7 +496,7 @@ const hyphenReplace = incPr => ($0,
     to = `<=${to}`
   }
 
-  return (`${from} ${to}`).trim()
+  return `${from} ${to}`.trim()
 }
 
 const testSet = (set, version, options) => {

Diff for: classes/semver.js

@@ -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')

Diff for: functions/coerce.js

@@ -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) {

Diff for: internal/re.js

@@ -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,

Diff for: package.json

@@ -37,7 +37,7 @@
     "range.bnf"
   ],
   "tap": {
-    "check-coverage": true,
+    "timeout": 30,
     "coverage-map": "map.js",
     "nyc-arg": [
       "--exclude",

Diff for: test/integration/whitespace.js

@@ -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()
+})

Diff for: test/internal/re.js

@@ -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) => {
@@ -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()
 })

Diff for: test/map.js

@@ -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()
+})