From 6d6767d4582b757b5aff1763931cc1765203ed06 Mon Sep 17 00:00:00 2001 From: Kris Reeves Date: Mon, 25 Nov 2024 10:49:24 -0800 Subject: [PATCH] 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` :) --- index.browser.js | 4 ++-- index.js | 8 ++++---- non-secure/index.js | 4 ++-- test/index.test.js | 7 +++++++ 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/index.browser.js b/index.browser.js index 9ec0a948..b8a1c8ce 100644 --- a/index.browser.js +++ b/index.browser.js @@ -47,11 +47,11 @@ export let customRandom = (alphabet, defaultSize, getRandom) => { } export let customAlphabet = (alphabet, size = 21) => - customRandom(alphabet, size, random) + customRandom(alphabet, size | 0, random) export let nanoid = (size = 21) => { let id = '' - let bytes = crypto.getRandomValues(new Uint8Array(size)) + let bytes = crypto.getRandomValues(new Uint8Array((size |= 0))) while (size--) { // Using the bitwise AND operator to "cap" the value of // the random byte from 255 to 63, in that way we can make sure diff --git a/index.js b/index.js index a1ce01c6..468b1eb8 100644 --- a/index.js +++ b/index.js @@ -25,8 +25,8 @@ function fillPool(bytes) { } export function 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) } @@ -70,8 +70,8 @@ export function customAlphabet(alphabet, size = 21) { } export function 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 78e522fa..3c3e43ba 100644 --- a/non-secure/index.js +++ b/non-secure/index.js @@ -11,7 +11,7 @@ export 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 @@ export let customAlphabet = (alphabet, defaultSize = 21) => { export 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/index.test.js b/test/index.test.js index 6c64f23d..cd52b588 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -57,6 +57,13 @@ for (let type of ['node', 'browser']) { } }) + test(`avoids pool pollution, infinite loop`, () => { + nanoid(2.1) + const second = nanoid() + const third = nanoid() + notEqual(second, third) + }) + test(`has flat distribution`, () => { let COUNT = 100 * 1000 let LENGTH = nanoid().length