From ea12f81b4be13cbf3ebeae9eb38cbaa8f45c12d1 Mon Sep 17 00:00:00 2001 From: isaacs Date: Thu, 6 May 2021 14:12:43 -0700 Subject: [PATCH] guard against locale-specific sorting Node will use the current environment's locale sorting for String.localeCompare. Thankfully, String.localeCompare can take a second argument to specify the locale. Up until Node 13, any valid argument provided to the function _may_ be respected if Node was compiled with Intl support, or would always be treated as `'en'` otherwise. The solution is to always set the locale to `'en'`. An alternative solution would be to compare strings based on byte order, but this does not provide ideal ergonomis. To verify this, the locale is set in the test environment to `'sk'`, which has a significantly different alphabet, including sorting words starting with `'ch'` _after_ words starting with `'d'`. Re: npm/cli#2829 Fix: #276 --- bin/license.js | 2 +- lib/add-rm-pkg-deps.js | 2 +- lib/arborist/build-ideal-tree.js | 6 +++--- lib/arborist/load-virtual.js | 4 ++-- lib/arborist/rebuild.js | 2 +- lib/audit-report.js | 2 +- lib/printable.js | 8 ++++---- lib/shrinkwrap.js | 2 +- lib/update-root-package-json.js | 2 +- lib/vuln.js | 6 +++--- lib/yarn-lock.js | 12 ++++++------ package.json | 4 +++- test/arborist/rebuild.js | 4 ++-- test/arborist/reify.js | 2 +- test/audit-report.js | 6 +++--- test/diff.js | 2 +- test/fixtures/index.js | 2 +- test/gather-dep-set.js | 2 +- test/inventory.js | 4 ++-- 19 files changed, 38 insertions(+), 36 deletions(-) diff --git a/bin/license.js b/bin/license.js index 4083ddc69..89d0d8790 100644 --- a/bin/license.js +++ b/bin/license.js @@ -22,7 +22,7 @@ a.loadVirtual().then(tree => { set.push([tree.inventory.query('license', license).size, license]) for (const [count, license] of set.sort((a, b) => - a[1] && b[1] ? b[0] - a[0] || a[1].localeCompare(b[1]) + a[1] && b[1] ? b[0] - a[0] || a[1].localeCompare(b[1], 'en') : a[1] ? -1 : b[1] ? 1 : 0)) diff --git a/lib/add-rm-pkg-deps.js b/lib/add-rm-pkg-deps.js index 25113cbed..f78a43319 100644 --- a/lib/add-rm-pkg-deps.js +++ b/lib/add-rm-pkg-deps.js @@ -75,7 +75,7 @@ const addSingle = ({pkg, spec, saveBundle, saveType, log}) => { // keep it sorted, keep it unique const bd = new Set(pkg.bundleDependencies || []) bd.add(spec.name) - pkg.bundleDependencies = [...bd].sort((a, b) => a.localeCompare(b)) + pkg.bundleDependencies = [...bd].sort((a, b) => a.localeCompare(b, 'en')) } } diff --git a/lib/arborist/build-ideal-tree.js b/lib/arborist/build-ideal-tree.js index 6176707c3..ade9bbf1a 100644 --- a/lib/arborist/build-ideal-tree.js +++ b/lib/arborist/build-ideal-tree.js @@ -764,7 +764,7 @@ This is a one-time fix-up, please be patient... // sort physically shallower deps up to the front of the queue, // because they'll affect things deeper in, then alphabetical this[_depsQueue].sort((a, b) => - (a.depth - b.depth) || a.path.localeCompare(b.path)) + (a.depth - b.depth) || a.path.localeCompare(b.path, 'en')) const node = this[_depsQueue].shift() const bd = node.package.bundleDependencies @@ -902,7 +902,7 @@ This is a one-time fix-up, please be patient... } const placed = tasks - .sort((a, b) => a.edge.name.localeCompare(b.edge.name)) + .sort((a, b) => a.edge.name.localeCompare(b.edge.name, 'en')) .map(({ edge, dep }) => this[_placeDep](dep, node, edge)) const promises = [] @@ -1147,7 +1147,7 @@ This is a one-time fix-up, please be patient... // we typically only install non-optional peers, but we have to // factor them into the peerSet so that we can avoid conflicts .filter(e => e.peer && !(e.valid && e.to)) - .sort(({name: a}, {name: b}) => a.localeCompare(b)) + .sort(({name: a}, {name: b}) => a.localeCompare(b, 'en')) for (const edge of peerEdges) { // already placed this one, and we're happy with it. diff --git a/lib/arborist/load-virtual.js b/lib/arborist/load-virtual.js index 2a222249d..a98ed23b2 100644 --- a/lib/arborist/load-virtual.js +++ b/lib/arborist/load-virtual.js @@ -159,12 +159,12 @@ module.exports = cls => class VirtualLoader extends cls { ...depsToEdges('peerOptional', peerOptional), ...lockWS, ].sort(([atype, aname], [btype, bname]) => - atype.localeCompare(btype) || aname.localeCompare(bname)) + atype.localeCompare(btype, 'en') || aname.localeCompare(bname, 'en')) const rootEdges = [...root.edgesOut.values()] .map(e => [e.type, e.name, e.spec]) .sort(([atype, aname], [btype, bname]) => - atype.localeCompare(btype) || aname.localeCompare(bname)) + atype.localeCompare(btype, 'en') || aname.localeCompare(bname, 'en')) if (rootEdges.length !== lockEdges.length) { // something added or removed diff --git a/lib/arborist/rebuild.js b/lib/arborist/rebuild.js index 390d3ce42..7cba1da00 100644 --- a/lib/arborist/rebuild.js +++ b/lib/arborist/rebuild.js @@ -14,7 +14,7 @@ const { } = require('@npmcli/node-gyp') const boolEnv = b => b ? '1' : '' -const sortNodes = (a, b) => (a.depth - b.depth) || a.path.localeCompare(b.path) +const sortNodes = (a, b) => (a.depth - b.depth) || a.path.localeCompare(b.path, 'en') const _build = Symbol('build') const _resetQueues = Symbol('resetQueues') diff --git a/lib/audit-report.js b/lib/audit-report.js index 9a0178c59..76387cde1 100644 --- a/lib/audit-report.js +++ b/lib/audit-report.js @@ -78,7 +78,7 @@ class AuditReport extends Map { } obj.vulnerabilities = vulnerabilities - .sort(([a], [b]) => a.localeCompare(b)) + .sort(([a], [b]) => a.localeCompare(b, 'en')) .reduce((set, [name, vuln]) => { set[name] = vuln return set diff --git a/lib/printable.js b/lib/printable.js index e611f55a4..ce764071d 100644 --- a/lib/printable.js +++ b/lib/printable.js @@ -46,14 +46,14 @@ class ArboristNode { // edgesOut sorted by name if (tree.edgesOut.size) { this.edgesOut = new Map([...tree.edgesOut.entries()] - .sort(([a], [b]) => a.localeCompare(b)) + .sort(([a], [b]) => a.localeCompare(b, 'en')) .map(([name, edge]) => [name, new EdgeOut(edge)])) } // edgesIn sorted by location if (tree.edgesIn.size) { this.edgesIn = new Set([...tree.edgesIn] - .sort((a, b) => a.from.location.localeCompare(b.from.location)) + .sort((a, b) => a.from.location.localeCompare(b.from.location, 'en')) .map(edge => new EdgeIn(edge))) } @@ -65,14 +65,14 @@ class ArboristNode { // fsChildren sorted by path if (tree.fsChildren.size) { this.fsChildren = new Set([...tree.fsChildren] - .sort(({path: a}, {path: b}) => a.localeCompare(b)) + .sort(({path: a}, {path: b}) => a.localeCompare(b, 'en')) .map(tree => printableTree(tree, path))) } // children sorted by name if (tree.children.size) { this.children = new Map([...tree.children.entries()] - .sort(([a], [b]) => a.localeCompare(b)) + .sort(([a], [b]) => a.localeCompare(b, 'en')) .map(([name, tree]) => [name, printableTree(tree, path)])) } } diff --git a/lib/shrinkwrap.js b/lib/shrinkwrap.js index d9065ffa0..cff9f0963 100644 --- a/lib/shrinkwrap.js +++ b/lib/shrinkwrap.js @@ -844,7 +844,7 @@ class Shrinkwrap { /* istanbul ignore next - sort calling order is indeterminate */ return aloc.length > bloc.length ? 1 : bloc.length > aloc.length ? -1 - : aloc[aloc.length - 1].localeCompare(bloc[bloc.length - 1]) + : aloc[aloc.length - 1].localeCompare(bloc[bloc.length - 1], 'en') })[0] const res = consistentResolve(node.resolved, this.path, this.path, true) diff --git a/lib/update-root-package-json.js b/lib/update-root-package-json.js index 4a88707b9..57ec41424 100644 --- a/lib/update-root-package-json.js +++ b/lib/update-root-package-json.js @@ -18,7 +18,7 @@ const orderDeps = (pkg) => { for (const type of depTypes) { if (pkg && pkg[type]) { pkg[type] = Object.keys(pkg[type]) - .sort((a, b) => a.localeCompare(b)) + .sort((a, b) => a.localeCompare(b, 'en')) .reduce((res, key) => { res[key] = pkg[type][key] return res diff --git a/lib/vuln.js b/lib/vuln.js index 2561bc806..5b1d1dc1a 100644 --- a/lib/vuln.js +++ b/lib/vuln.js @@ -106,12 +106,12 @@ class Vuln { vulnerableVersions: undefined, id: undefined, }).sort((a, b) => - String(a.source || a).localeCompare(String(b.source || b))), + String(a.source || a).localeCompare(String(b.source || b, 'en'))), effects: [...this.effects].map(v => v.name) - .sort(/* istanbul ignore next */(a, b) => a.localeCompare(b)), + .sort(/* istanbul ignore next */(a, b) => a.localeCompare(b, 'en')), range: this.simpleRange, nodes: [...this.nodes].map(n => n.location) - .sort(/* istanbul ignore next */(a, b) => a.localeCompare(b)), + .sort(/* istanbul ignore next */(a, b) => a.localeCompare(b, 'en')), fixAvailable: this[_fixAvailable], } } diff --git a/lib/yarn-lock.js b/lib/yarn-lock.js index 14c7691f1..e237cc5c6 100644 --- a/lib/yarn-lock.js +++ b/lib/yarn-lock.js @@ -34,7 +34,7 @@ const {breadth} = require('treeverse') // sort a key/value object into a string of JSON stringified keys and vals const sortKV = obj => Object.keys(obj) - .sort((a, b) => a.localeCompare(b)) + .sort((a, b) => a.localeCompare(b, 'en')) .map(k => ` ${JSON.stringify(k)} ${JSON.stringify(obj[k])}`) .join('\n') @@ -165,7 +165,7 @@ class YarnLock { toString () { return prefix + [...new Set([...this.entries.values()])] .map(e => e.toString()) - .sort((a, b) => a.localeCompare(b)).join('\n\n') + '\n' + .sort((a, b) => a.localeCompare(b, 'en')).join('\n\n') + '\n' } fromTree (tree) { @@ -175,7 +175,7 @@ class YarnLock { tree, visit: node => this.addEntryFromNode(node), getChildren: node => [...node.children.values(), ...node.fsChildren] - .sort((a, b) => a.depth - b.depth || a.name.localeCompare(b.name)), + .sort((a, b) => a.depth - b.depth || a.name.localeCompare(b.name, 'en')), }) return this } @@ -183,7 +183,7 @@ class YarnLock { addEntryFromNode (node) { const specs = [...node.edgesIn] .map(e => `${node.name}@${e.spec}`) - .sort((a, b) => a.localeCompare(b)) + .sort((a, b) => a.localeCompare(b, 'en')) // Note: // yarn will do excessive duplication in a case like this: @@ -309,7 +309,7 @@ class YarnLockEntry { toString () { // sort objects to the bottom, then alphabetical return ([...this[_specs]] - .sort((a, b) => a.localeCompare(b)) + .sort((a, b) => a.localeCompare(b, 'en')) .map(JSON.stringify).join(', ') + ':\n' + Object.getOwnPropertyNames(this) @@ -318,7 +318,7 @@ class YarnLockEntry { (a, b) => /* istanbul ignore next - sort call order is unpredictable */ (typeof this[a] === 'object') === (typeof this[b] === 'object') - ? a.localeCompare(b) + ? a.localeCompare(b, 'en') : typeof this[a] === 'object' ? 1 : -1) .map(prop => typeof this[prop] !== 'object' diff --git a/package.json b/package.json index ebc84f6fc..b003cf20d 100644 --- a/package.json +++ b/package.json @@ -74,11 +74,13 @@ "bin": { "arborist": "bin/index.js" }, + "//": "sk test-env locale to catch locale-specific sorting", "tap": { "after": "test/fixtures/cleanup.js", "coverage-map": "map.js", "test-env": [ - "NODE_OPTIONS=--no-warnings" + "NODE_OPTIONS=--no-warnings", + "LC_ALL=sk" ], "node-arg": [ "--no-warnings", diff --git a/test/arborist/rebuild.js b/test/arborist/rebuild.js index ba75d8edb..beba11779 100644 --- a/test/arborist/rebuild.js +++ b/test/arborist/rebuild.js @@ -181,7 +181,7 @@ t.test('verify dep flags in script environments', async t => { // don't include path or env, because that's going to be platform-specific const saved = [...arb.scriptsRun] .sort(({path: patha, event: eventa}, {path: pathb, event: eventb}) => - patha.localeCompare(pathb) || eventa.localeCompare(eventb)) + patha.localeCompare(pathb, 'en') || eventa.localeCompare(eventb, 'en')) .map(({pkg, event, cmd, code, signal, stdout, stderr}) => ({pkg, event, cmd, code, signal, stdout, stderr})) t.matchSnapshot(saved, 'saved script results') @@ -191,7 +191,7 @@ t.test('verify dep flags in script environments', async t => { t.strictSame(flags, fs.readFileSync(file, 'utf8').split('\n'), pkg) } t.strictSame(checkLogs().sort((a, b) => - a[2].localeCompare(b[2]) || (typeof a[4] === 'string' ? -1 : 1)), [ + a[2].localeCompare(b[2], 'en') || (typeof a[4] === 'string' ? -1 : 1)), [ ['info', 'run', 'devdep@1.0.0', 'postinstall', 'node_modules/devdep', 'node ../../env.js'], ['info', 'run', 'devdep@1.0.0', 'postinstall', {code: 0, signal: null}], ['info', 'run', 'devopt@1.0.0', 'postinstall', 'node_modules/devopt', 'node ../../env.js'], diff --git a/test/arborist/reify.js b/test/arborist/reify.js index 0fd20fd99..6f0af6e32 100644 --- a/test/arborist/reify.js +++ b/test/arborist/reify.js @@ -204,7 +204,7 @@ t.test('omit peer deps', t => { .then(() => { process.removeListener('time', onTime) process.removeListener('timeEnd', onTimeEnd) - finishedTimers.sort((a, b) => a.localeCompare(b)) + finishedTimers.sort((a, b) => a.localeCompare(b, 'en')) t.matchSnapshot(finishedTimers, 'finished timers') t.strictSame(timers, {}, 'should have no timers in progress now') }) diff --git a/test/audit-report.js b/test/audit-report.js index 692c5c0eb..775552069 100644 --- a/test/audit-report.js +++ b/test/audit-report.js @@ -23,14 +23,14 @@ const newArb = (path, opts = {}) => new Arborist({path, registry, cache, ...opts const sortReport = report => { const entries = Object.entries(report.vulnerabilities) - const vulns = entries.sort(([a], [b]) => a.localeCompare(b)) + const vulns = entries.sort(([a], [b]) => a.localeCompare(b, 'en')) .map(([name, vuln]) => [ name, { ...vuln, via: (vuln.via || []).sort((a, b) => - String(a.source || a).localeCompare(String(b.source || b))), - effects: (vuln.effects || []).sort((a, b) => a.localeCompare(b)), + String(a.source || a).localeCompare(String(b.source || b, 'en'))), + effects: (vuln.effects || []).sort((a, b) => a.localeCompare(b, 'en')), }, ]) report.vulnerabilities = vulns.reduce((set, [k, v]) => { diff --git a/test/diff.js b/test/diff.js index 7a82a3d97..37a27522e 100644 --- a/test/diff.js +++ b/test/diff.js @@ -28,7 +28,7 @@ const formatDiff = obj => removed: obj.removed.map(d => normalizePath(d.path)), children: [...obj.children] .map(formatDiff) - .sort((a, b) => path(a).localeCompare(path(b))), + .sort((a, b) => path(a).localeCompare(path(b, 'en'))), }) t.formatSnapshot = obj => format(formatDiff(obj), { sort: false }) diff --git a/test/fixtures/index.js b/test/fixtures/index.js index 529177179..e2965721f 100644 --- a/test/fixtures/index.js +++ b/test/fixtures/index.js @@ -167,7 +167,7 @@ const setup = () => { `### BEGIN IGNORED SYMLINKS ### ### this list is generated automatically, do not edit directly ### update it by running \`node test/fixtures/index.js\` -${links.sort((a,b) => a.localeCompare(b)).join('\n')} +${links.sort((a,b) => a.localeCompare(b, 'en')).join('\n')} ### END IGNORED SYMLINKS ###`) writeFileSync(gifile, gitignore) } diff --git a/test/gather-dep-set.js b/test/gather-dep-set.js index f22ab1d41..877e41900 100644 --- a/test/gather-dep-set.js +++ b/test/gather-dep-set.js @@ -81,7 +81,7 @@ const tree = new Node({ const normalizePath = path => path.replace(/[A-Z]:/, '').replace(/\\/g, '/') const printSet = set => [...set] - .sort((a, b) => a.name.localeCompare(b.name)) + .sort((a, b) => a.name.localeCompare(b.name, 'en')) .map(n => n.location) const cwd = normalizePath(process.cwd()) diff --git a/test/inventory.js b/test/inventory.js index 638cd911d..8f985274c 100644 --- a/test/inventory.js +++ b/test/inventory.js @@ -16,7 +16,7 @@ t.test('basic operations', t => { i.get('y'), ], 'filter returns an iterable of all matching nodes') - t.same([...i.query('license')].sort((a, b) => String(a).localeCompare(String(b))), + t.same([...i.query('license')].sort((a, b) => String(a).localeCompare(String(b, 'en'))), ['ISC', 'MIT', undefined]) t.same([...i.query('license', 'MIT')], [ { location: 'x', name: 'x', package: { license: 'MIT', funding: 'foo' }}, @@ -28,7 +28,7 @@ t.test('basic operations', t => { { location: 'x', name: 'x', package: { license: 'MIT', funding: 'foo'}}, { location: 'y', name: 'x', package: { license: 'ISC', funding: { url: 'foo' } }}, ], 'can query by name') - t.same([...i.query('funding')].sort((a, b) => String(a).localeCompare(String(b))), + t.same([...i.query('funding')].sort((a, b) => String(a).localeCompare(String(b, 'en'))), ['bar', 'foo', undefined]) t.same([...i.query('funding', 'foo')], [ { location: 'x', name: 'x', package: { license: 'MIT', funding: 'foo' } },