From d643045f40d6dc8afa000a644d857da1436ed08c Mon Sep 17 00:00:00 2001 From: Kris Reeves Date: Tue, 26 Nov 2024 03:31:28 -0800 Subject: [PATCH] Fix pool pollution, infinite loop (#510) * Fix pool pollution, infinite loop When nanoid is called with a fractional value, there were a number of undesirable effects: - in browser and non-secure, the code infinite loops on `while (size--)` - in node, the value of poolOffset becomes fractional, causing calls to nanoid to return zeroes until the pool is next filled: when `i` is initialized to `poolOffset`, `pool[i] & 63` -> `undefined & 63` -> `0` - if the first call in node is a fractional argument, the initial buffer allocation fails with an error I chose `|0` to cast to a signed integer primarily because that has a slightly better outcome in the third case above: if the first call is negative (e.g. `nanoid(-1)`) then Node will throw an error for an invalid Buffer size, rather than attempting to allocate a buffer of size `2**32-1`. It's also more compact than `>>>0`, which would be necessary to cast to an unsigned integer. I don't _think_ there is a use case for generating ids longer than `2**31-1` :) The browser code is structured in such a way that casting `size` in `customRandom` succinctly isn't readily feasible. I chose to cast it at the line `let j = step | 0` since casting defaultSize would not fix the infinite loop in all cases, and the other use of defaultSize is to define the step length which is already shown to be fractional and gets cast to an integer with `~` anyway. As for the `nanoid` function, `new Uint8Array(size)` ignores the fractional part, and `size` doesn't get used further - the function instead calls reduce over the typed array. In the Node/native async customAlphabet variant, I chose to convert the `id.length === size` check to `id.length >= size`, which handles the fractional case and avoids the infinite loop; `size` is not used for anything else there. * `const` -> `let` --- async/index.browser.js | 4 ++-- async/index.js | 4 ++-- async/index.native.js | 4 ++-- index.browser.js | 2 +- index.js | 8 ++++---- non-secure/index.js | 4 ++-- test/async.test.js | 9 ++++++++- test/index.test.js | 9 ++++++++- test/non-secure.test.js | 18 +++++++++++++++++- test/react-native-polyfill.test.js | 10 +++++++++- 10 files changed, 55 insertions(+), 17 deletions(-) diff --git a/async/index.browser.js b/async/index.browser.js index 402e2f4e..80d18716 100644 --- a/async/index.browser.js +++ b/async/index.browser.js @@ -29,7 +29,7 @@ let customAlphabet = (alphabet, defaultSize = 21) => { while (true) { let bytes = crypto.getRandomValues(new Uint8Array(step)) // A compact alternative for `for (var i = 0; i < step; i++)`. - let i = step + let i = step | 0 while (i--) { // Adding `|| ''` refuses a random byte that exceeds the alphabet size. id += alphabet[bytes[i] & mask] || '' @@ -41,7 +41,7 @@ let customAlphabet = (alphabet, defaultSize = 21) => { let nanoid = async (size = 21) => { let id = '' - let bytes = crypto.getRandomValues(new Uint8Array(size)) + let bytes = crypto.getRandomValues(new Uint8Array((size |= 0))) // A compact alternative for `for (var i = 0; i < step; i++)`. while (size--) { diff --git a/async/index.js b/async/index.js index 1e634e91..470d381f 100644 --- a/async/index.js +++ b/async/index.js @@ -45,7 +45,7 @@ let customAlphabet = (alphabet, defaultSize = 21) => { while (i--) { // Adding `|| ''` refuses a random byte that exceeds the alphabet size. id += alphabet[bytes[i] & mask] || '' - if (id.length === size) return id + if (id.length >= size) return id } return tick(id, size) }) @@ -54,7 +54,7 @@ let customAlphabet = (alphabet, defaultSize = 21) => { } let nanoid = (size = 21) => - random(size).then(bytes => { + random((size |= 0)).then(bytes => { let id = '' // A compact alternative for `for (var i = 0; i < step; i++)`. while (size--) { diff --git a/async/index.native.js b/async/index.native.js index 0ebdacd0..239ef6fc 100644 --- a/async/index.native.js +++ b/async/index.native.js @@ -31,7 +31,7 @@ let customAlphabet = (alphabet, defaultSize = 21) => { while (i--) { // Adding `|| ''` refuses a random byte that exceeds the alphabet size. id += alphabet[bytes[i] & mask] || '' - if (id.length === size) return id + if (id.length >= size) return id } return tick(id, size) }) @@ -40,7 +40,7 @@ let customAlphabet = (alphabet, defaultSize = 21) => { } let nanoid = (size = 21) => - random(size).then(bytes => { + random((size |= 0)).then(bytes => { let id = '' // A compact alternative for `for (var i = 0; i < step; i++)`. while (size--) { diff --git a/index.browser.js b/index.browser.js index 7ab1562c..4f3c4b4e 100644 --- a/index.browser.js +++ b/index.browser.js @@ -34,7 +34,7 @@ let customRandom = (alphabet, defaultSize, getRandom) => { while (true) { let bytes = getRandom(step) // A compact alternative for `for (var i = 0; i < step; i++)`. - let j = step + let j = step | 0 while (j--) { // Adding `|| ''` refuses a random byte that exceeds the alphabet size. id += alphabet[bytes[j] & mask] || '' diff --git a/index.js b/index.js index 977f07b0..5a8a0751 100644 --- a/index.js +++ b/index.js @@ -23,8 +23,8 @@ let fillPool = bytes => { } let random = bytes => { - // `-=` convert `bytes` to number to prevent `valueOf` abusing - fillPool((bytes -= 0)) + // `|=` convert `bytes` to number to prevent `valueOf` abusing and pool pollution + fillPool((bytes |= 0)) return pool.subarray(poolOffset - bytes, poolOffset) } @@ -67,8 +67,8 @@ let customAlphabet = (alphabet, size = 21) => customRandom(alphabet, size, random) let nanoid = (size = 21) => { - // `-=` convert `size` to number to prevent `valueOf` abusing - fillPool((size -= 0)) + // `|=` convert `size` to number to prevent `valueOf` abusing and pool pollution + fillPool((size |= 0)) let id = '' // We are reading directly from the random pool to avoid creating new array for (let i = poolOffset - size; i < poolOffset; i++) { diff --git a/non-secure/index.js b/non-secure/index.js index 947d84fa..d51fcb6c 100644 --- a/non-secure/index.js +++ b/non-secure/index.js @@ -11,7 +11,7 @@ let customAlphabet = (alphabet, defaultSize = 21) => { return (size = defaultSize) => { let id = '' // A compact alternative for `for (var i = 0; i < step; i++)`. - let i = size + let i = size | 0 while (i--) { // `| 0` is more compact and faster than `Math.floor()`. id += alphabet[(Math.random() * alphabet.length) | 0] @@ -23,7 +23,7 @@ let customAlphabet = (alphabet, defaultSize = 21) => { let nanoid = (size = 21) => { let id = '' // A compact alternative for `for (var i = 0; i < step; i++)`. - let i = size + let i = size | 0 while (i--) { // `| 0` is more compact and faster than `Math.floor()`. id += urlAlphabet[(Math.random() * 64) | 0] diff --git a/test/async.test.js b/test/async.test.js index 1ffd789a..7c01d357 100644 --- a/test/async.test.js +++ b/test/async.test.js @@ -1,6 +1,6 @@ let { suite } = require('uvu') let { spy } = require('nanospy') -let { is, match, ok } = require('uvu/assert') +let { is, match, ok, not } = require('uvu/assert') let crypto = require('crypto') global.crypto = { @@ -55,6 +55,13 @@ for (let type of ['node', 'browser']) { ) }) + nanoidSuite('avoids pool pollution, infinite loop', async () => { + await nanoid(2.1) + let second = await nanoid() + let third = await nanoid() + not.equal(second, third) + }) + nanoidSuite('changes ID length', async () => { let id = await nanoid(10) is(id.length, 10) diff --git a/test/index.test.js b/test/index.test.js index 9bc3350c..3dfd563a 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -1,5 +1,5 @@ let { test } = require('uvu') -let { is, ok, equal, match } = require('uvu/assert') +let { is, ok, equal, match, not } = require('uvu/assert') let browser = require('../index.browser.js') let node = require('../index.js') @@ -34,6 +34,13 @@ for (let type of ['node', 'browser']) { } }) + test(`${type} / nanoid / avoids pool pollution, infinite loop`, () => { + nanoid(2.1) + let second = nanoid() + let third = nanoid() + not.equal(second, third) + }) + test(`${type} / nanoid / changes ID length`, () => { is(nanoid(10).length, 10) }) diff --git a/test/non-secure.test.js b/test/non-secure.test.js index 6b6117b4..f952da64 100644 --- a/test/non-secure.test.js +++ b/test/non-secure.test.js @@ -1,5 +1,5 @@ let { test } = require('uvu') -let { is, match, ok } = require('uvu/assert') +let { is, match, ok, not } = require('uvu/assert') let { nanoid, customAlphabet } = require('../non-secure') let { urlAlphabet } = require('..') @@ -56,6 +56,13 @@ test('nanoid / has flat distribution', () => { ok(max - min <= 0.05) }) +test('nanoid / avoids pool pollution, infinite loop', () => { + nanoid(2.1) + let second = nanoid() + let third = nanoid() + not.equal(second, third) +}) + test('customAlphabet / has options', () => { let nanoidA = customAlphabet('a', 5) is(nanoidA(), 'aaaaa') @@ -88,4 +95,13 @@ test('customAlphabet / has flat distribution', () => { ok(max - min <= 0.05) }) +test('customAlphabet / avoids pool pollution, infinite loop', () => { + let ALPHABET = 'abcdefghijklmnopqrstuvwxyz' + let nanoid2 = customAlphabet(ALPHABET) + nanoid2(2.1) + let second = nanoid2() + let third = nanoid2() + not.equal(second, third) +}) + test.run() diff --git a/test/react-native-polyfill.test.js b/test/react-native-polyfill.test.js index d6615ed7..d96e2168 100644 --- a/test/react-native-polyfill.test.js +++ b/test/react-native-polyfill.test.js @@ -1,5 +1,5 @@ let { test } = require('uvu') -let { is } = require('uvu/assert') +let { is, not } = require('uvu/assert') test.before(() => { global.navigator = { @@ -27,4 +27,12 @@ test('works with polyfill', () => { is(typeof nanoid(), 'string') }) +test('nanoid / avoids pool pollution, infinite loop', () => { + let { nanoid } = require('../index.browser') + nanoid(2.1) + let second = nanoid() + let third = nanoid() + not.equal(second, third) +}) + test.run()