Skip to content

Commit

Permalink
fix: better randomInt, and arrayShuffle
Browse files Browse the repository at this point in the history
  • Loading branch information
peter-sanderson committed Oct 15, 2024
1 parent fb7dc6a commit 42529a9
Show file tree
Hide file tree
Showing 25 changed files with 120 additions and 31 deletions.
4 changes: 2 additions & 2 deletions packages/coinjoin/src/client/round/endedRound.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { enumUtils, getRandomNumberInRange } from '@trezor/utils';
import { enumUtils, getWeakRandomNumberInRange } from '@trezor/utils';

import type { CoinjoinRound, CoinjoinRoundOptions } from '../CoinjoinRound';
import { EndRoundState, WabiSabiProtocolErrorCode } from '../../enums';
Expand Down Expand Up @@ -56,7 +56,7 @@ export const ended = (round: CoinjoinRound, { logger, network }: CoinjoinRoundOp
// repeated input-registration will tell if they are really banned,
// make sure that addresses registered in round are recycled (reset Infinity sentence)
const minute = 60 * 1000;
const sentenceEnd = getRandomNumberInRange(5 * minute, 10 * minute);
const sentenceEnd = getWeakRandomNumberInRange(5 * minute, 10 * minute);
[...inputs, ...addresses].forEach(vinvout =>
prison.detain(vinvout, {
sentenceEnd,
Expand Down
4 changes: 2 additions & 2 deletions packages/coinjoin/src/client/round/inputRegistration.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getRandomNumberInRange } from '@trezor/utils';
import { getWeakRandomNumberInRange } from '@trezor/utils';

import * as coordinator from '../coordinator';
import * as middleware from '../middleware';
Expand Down Expand Up @@ -56,7 +56,7 @@ const registerInput = async (
// setup random delay for registration request. we want each input to be registered in different time as different TOR identity
// note that this may cause that the input will not be registered if phase change before expected deadline
const deadline = round.phaseDeadline - Date.now() - ROUND_SELECTION_REGISTRATION_OFFSET;
const delay = deadline > 0 ? getRandomNumberInRange(0, deadline) : 0;
const delay = deadline > 0 ? getWeakRandomNumberInRange(0, deadline) : 0;
logger.info(
`Trying to register ~~${input.outpoint}~~ to ~~${round.id}~~ with delay ${delay}ms and deadline ${round.phaseDeadline}`,
);
Expand Down
4 changes: 2 additions & 2 deletions packages/coinjoin/src/utils/roundUtils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { bufferutils, Transaction, Network } from '@trezor/utxo-lib';
import { getRandomNumberInRange } from '@trezor/utils';
import { getWeakRandomNumberInRange } from '@trezor/utils';

import {
COORDINATOR_FEE_RATE_FALLBACK,
Expand Down Expand Up @@ -88,7 +88,7 @@ export const scheduleDelay = (
// and at most 1 sec before the calculated max (so there's room for randomness)
const min = clamp(minimumDelay, 0, max - 1000);

return getRandomNumberInRange(min, max);
return getWeakRandomNumberInRange(min, max);
};

// NOTE: deadlines are not accurate. phase may change earlier
Expand Down
4 changes: 2 additions & 2 deletions packages/coinjoin/tests/client/CoinjoinRound.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ describe(`CoinjoinRound`, () => {

it('onPhaseChange lock cool off resolved', async () => {
const delayMock = jest
.spyOn(trezorUtils, 'getRandomNumberInRange')
.spyOn(trezorUtils, 'getWeakRandomNumberInRange')
.mockImplementation(() => 800);

const constantsMock = jest
Expand Down Expand Up @@ -396,7 +396,7 @@ describe(`CoinjoinRound`, () => {

it('unregisterAccount when round is locked', async () => {
const delayMock = jest
.spyOn(trezorUtils, 'getRandomNumberInRange')
.spyOn(trezorUtils, 'getWeakRandomNumberInRange')
.mockImplementation(() => 800);

const constantsMock = jest
Expand Down
8 changes: 4 additions & 4 deletions packages/coinjoin/tests/client/transactionSigning.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { networks } from '@trezor/utxo-lib';
import { getRandomNumberInRange } from '@trezor/utils';
import { getWeakRandomNumberInRange } from '@trezor/utils';

import { transactionSigning } from '../../src/client/round/transactionSigning';
import { createServer } from '../mocks/server';
Expand Down Expand Up @@ -529,7 +529,7 @@ describe('transactionSigning signature delay', () => {
);

// signature is sent in range 17-67 sec. (resolve time is less than 50 sec TX_SIGNING_DELAY)
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(17000, 67000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(17000, 67000);
expect(response.isSignedSuccessfully()).toBe(true);
});

Expand Down Expand Up @@ -558,7 +558,7 @@ describe('transactionSigning signature delay', () => {
);

// signature is sent in range 0-46.21 sec. (resolve time is greater than 50 sec of TX_SIGNING_DELAY)
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(0, 46210);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(0, 46210);
expect(response.isSignedSuccessfully()).toBe(true);
});

Expand Down Expand Up @@ -588,7 +588,7 @@ describe('transactionSigning signature delay', () => {
);

// signature is sent in default range 0-1 sec.
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(0, 1000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(0, 1000);
expect(response.isSignedSuccessfully()).toBe(true);
});
});
20 changes: 10 additions & 10 deletions packages/coinjoin/tests/utils/roundUtils.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getRandomNumberInRange } from '@trezor/utils';
import { getWeakRandomNumberInRange } from '@trezor/utils';

import {
getCommitmentData,
Expand Down Expand Up @@ -150,38 +150,38 @@ describe('roundUtils', () => {

// default (no min, no max) range 0-10 sec.
resultInRange(scheduleDelay(60000), 0, 10000);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(0, 10000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(0, 10000);

// range 3-10sec.
resultInRange(scheduleDelay(20000, 3000), 3000, 10000);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(3000, 10000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(3000, 10000);

// deadlineOffset < 0, range 0-1 sec.
resultInRange(scheduleDelay(1000, 3000), 0, 1000);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(0, 1000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(0, 1000);

// deadline < min, range 9-10 sec.
resultInRange(scheduleDelay(60000, 61000), 9000, 10000);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(9000, 10000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(9000, 10000);

// deadline < min && deadline < max, range 49-50 sec.
resultInRange(scheduleDelay(60000, 61000, 62000), 49000, 50000);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(49000, 50000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(49000, 50000);

// deadline > min && deadline < max, range 3-20 sec.
resultInRange(scheduleDelay(30000, 3000, 50000), 3000, 20000);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(3000, 20000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(3000, 20000);

// min < 0 && deadline < max && deadlineOffset > 0, range 0-2.5 sec.
resultInRange(scheduleDelay(12500, -3000, 50000), 0, 2500);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(0, 2500);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(0, 2500);

// min < 0 && max < 0 && deadlineOffset > 0, range 0-1 sec.
resultInRange(scheduleDelay(12500, -10000, -5000), 0, 1000);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(0, 1000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(0, 1000);

// min < 0 && max < 0 && deadlineOffset < 0, range 0-1 sec.
resultInRange(scheduleDelay(7500, -10000, -5000), 0, 1000);
expect(getRandomNumberInRange).toHaveBeenLastCalledWith(0, 1000);
expect(getWeakRandomNumberInRange).toHaveBeenLastCalledWith(0, 1000);
});
});
1 change: 1 addition & 0 deletions packages/connect-popup/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"@trezor/device-utils": "workspace:*",
"@trezor/transport": "workspace:*",
"@trezor/urls": "workspace:*",
"crypto-browserify": "^3.12.0",
"eth-phishing-detect": "^1.2.0",
"react-dom": "18.2.0"
},
Expand Down
5 changes: 5 additions & 0 deletions packages/connect-popup/webpack/prod.webpack.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ const config: webpack.Configuration = {
extensions: ['.ts', '.tsx', '.js', '.jsx'],
modules: ['node_modules'],
mainFields: ['browser', 'module', 'main'],
fallback: {
// Polyfills crypto API for Node.js libraries in the browser. 'crypto' does not run without 'stream'
crypto: require.resolve('crypto-browserify'),
stream: require.resolve('stream-browserify'),
},
},
plugins: [
new DefinePlugin({
Expand Down
3 changes: 2 additions & 1 deletion packages/connect-web/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@
"dependencies": {
"@trezor/connect": "workspace:*",
"@trezor/connect-common": "workspace:*",
"@trezor/utils": "workspace:*"
"@trezor/utils": "workspace:*",
"crypto-browserify": "^3.12.0"
},
"devDependencies": {
"@babel/preset-typescript": "^7.24.7",
Expand Down
7 changes: 7 additions & 0 deletions packages/connect-web/webpack/dev.webpack.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ const dev = {
libraryTarget: 'umd',
libraryExport: 'default',
},
resolve: {
fallback: {
// Polyfills crypto API for NodeJS libraries in the browser. 'crypto' does not run without 'stream'
crypto: require.resolve('crypto-browserify'),
stream: require.resolve('stream-browserify'),
},
},
plugins: [
// connect-web dev needs to be served from https
// to allow injection in 3rd party builds using trezor-connect-src param
Expand Down
4 changes: 4 additions & 0 deletions packages/connect-web/webpack/prod.webpack.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ const config: webpack.Configuration = {
mainFields: ['browser', 'module', 'main'],
extensions: ['.ts', '.js'],
fallback: {
// Polyfills crypto API for Node.js libraries in the browser. 'crypto' does not run without 'stream'
crypto: require.resolve('crypto-browserify'),
stream: require.resolve('stream-browserify'),

fs: false, // ignore "fs" import in markdown-it-imsize
path: false, // ignore "path" import in markdown-it-imsize
},
Expand Down
1 change: 1 addition & 0 deletions packages/connect-webextension/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
"@trezor/connect-common": "workspace:*",
"@trezor/connect-web": "workspace:*",
"@trezor/utils": "workspace:*",
"crypto-browserify": "^3.12.0",
"events": "^3.3.0"
},
"devDependencies": {
Expand Down
5 changes: 5 additions & 0 deletions packages/connect-webextension/webpack/prod.webpack.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ const config: webpack.Configuration = {
modules: ['node_modules'],
mainFields: ['browser', 'module', 'main'],
extensions: ['.ts', '.js'],
fallback: {
// Polyfills crypto API for Node.js libraries in the browser. 'crypto' does not run without 'stream'
crypto: require.resolve('crypto-browserify'),
stream: require.resolve('stream-browserify'),
},
},
performance: {
hints: false,
Expand Down
2 changes: 1 addition & 1 deletion packages/suite-build/configs/base.webpack.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ const config: webpack.Configuration = {
src: path.resolve(__dirname, '../../suite/src/'),
},
fallback: {
// Polyfills crypto API for NodeJS libraries in the browser. 'crypto' does not run without 'stream'
// Polyfills crypto API for Node.js libraries in the browser. 'crypto' does not run without 'stream'
crypto: require.resolve('crypto-browserify'),
stream: require.resolve('stream-browserify'),
// Not required
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { useState } from 'react';
import styled from 'styled-components';
import { Card, Column, variables } from '@trezor/components';
import { Translation } from 'src/components/suite';
import { getRandomNumberInRange } from '@trezor/utils';
import { getWeakRandomNumberInRange } from '@trezor/utils';
import { typography } from '@trezor/theme';

const NoResults = styled.div`
Expand Down Expand Up @@ -46,7 +46,7 @@ const getTip = (num: number) => {
};

export const NoSearchResults = () => {
const [tip] = useState(getRandomNumberInRange(1, 10));
const [tip] = useState(getWeakRandomNumberInRange(1, 10));

return (
<Card>
Expand Down
1 change: 1 addition & 0 deletions packages/transport-bridge/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
"@trezor/theme": "workspace:*",
"@trezor/transport": "workspace:*",
"@trezor/utils": "workspace:*",
"crypto-browserify": "^3.12.0",
"json-stable-stringify": "^1.1.1",
"react": "18.2.0",
"react-dom": "18.2.0",
Expand Down
5 changes: 5 additions & 0 deletions packages/transport-bridge/webpack/ui.webpack.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ const config: webpack.Configuration = {
extensions: ['.ts', '.tsx', '.js', '.jsx'],
modules: ['node_modules'],
mainFields: ['browser', 'module', 'main'],
fallback: {
// Polyfills crypto API for Node.js libraries in the browser. 'crypto' does not run without 'stream'
crypto: require.resolve('crypto-browserify'),
stream: require.resolve('stream-browserify'),
},
},
plugins: [
new HtmlWebpackPlugin({
Expand Down
3 changes: 2 additions & 1 deletion packages/transport-test/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"test:e2e:new-bridge:hw": "USE_HW=true USE_NODE_BRIDGE=true yarn test:e2e:bridge",
"test:e2e:new-bridge:emu": "USE_HW=false USE_NODE_BRIDGE=true yarn test:e2e:bridge",
"build:e2e:api:node": "yarn esbuild ./e2e/api/api.test.ts --bundle --outfile=./e2e/dist/api.test.node.js --platform=node --target=node18 --external:usb",
"build:e2e:api:browser": "yarn esbuild ./e2e/api/api.test.ts --bundle --outfile=./e2e/dist/api.test.browser.js --platform=browser --external:usb && cp e2e/ui/api.test.html e2e/dist/index.html",
"build:e2e:api:browser": "yarn esbuild ./e2e/api/api.test.ts --bundle --alias:crypto=crypto-browserify --alias:stream=stream-browserify --outfile=./e2e/dist/api.test.browser.js --platform=browser --external:usb && cp e2e/ui/api.test.html e2e/dist/index.html",
"test:e2e:api:node:hw": "yarn build:e2e:api:node && node ./e2e/dist/api.test.node.js",
"test:e2e:api:browser:hw": "yarn build:e2e:api:browser && npx http-serve ./e2e/dist"
},
Expand All @@ -25,6 +25,7 @@
"@trezor/trezor-user-env-link": "workspace:^",
"@trezor/utils": "workspace:*",
"buffer": "^6.0.3",
"crypto-browserify": "^3.12.0",
"esbuild": "^0.23.1",
"jest": "^29.7.0",
"jest-extended": "^4.0.2",
Expand Down
4 changes: 3 additions & 1 deletion packages/utils/src/arrayShuffle.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { getRandomInt } from './getRandomInt';

/**
* Implementation of the Fisher-Yates shuffle algorithm.
* The algorithm produces an unbiased permutation: every permutation is equally likely.
Expand All @@ -8,7 +10,7 @@
export const arrayShuffle = <T>(array: readonly T[]): T[] => {
const shuffled = array.slice();
for (let i = shuffled.length - 1; i > 0; i--) {
const j = Math.floor(Math.random() * (i + 1));
const j = getRandomInt(0, i);
[shuffled[i], shuffled[j]] = [shuffled[j], shuffled[i]];
}

Expand Down
17 changes: 17 additions & 0 deletions packages/utils/src/getRandomInt.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { randomBytes } from 'crypto';

/**
* Crypto.randomInt() function is not implemented by polyfill 'crypto-browserify'
* @see https://github.com/browserify/crypto-browserify/issues/224
*/
export const getRandomInt = (min: number, max: number) => {
if (min >= max) {
throw new RangeError(
`The value of "max" is out of range. It must be greater than the value of "min" (${min}). Received ${max}`,
);
}

const randomValue = parseInt(randomBytes(4).toString('hex'), 16);

return min + (randomValue % (max - min));
};
2 changes: 0 additions & 2 deletions packages/utils/src/getRandomNumberInRange.ts

This file was deleted.

5 changes: 5 additions & 0 deletions packages/utils/src/getWeakRandomNumberInRange.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
/**
* @deprecated Consider using `getRandomInt` which is cryptographically secure.
*/
export const getWeakRandomNumberInRange = (min: number, max: number) =>
Math.floor(Math.random() * (max - min + 1)) + min;
3 changes: 2 additions & 1 deletion packages/utils/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export * from './createTimeoutPromise';
export * from './getLocaleSeparators';
export * from './getMutex';
export * from './getNumberFromPixelString';
export * from './getRandomNumberInRange';
export * from './getWeakRandomNumberInRange';
export * from './getSynchronize';
export * from './getWeakRandomId';
export * from './hasUppercaseLetter';
Expand All @@ -40,6 +40,7 @@ export * from './topologicalSort';
export * from './truncateMiddle';
export * from './typedEventEmitter';
export * from './urlToOnion';
export * from './getRandomInt';
export * from './logs';
export * from './logsManager';
export * from './bigNumber';
Expand Down
30 changes: 30 additions & 0 deletions packages/utils/tests/getRandomInt.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { randomInt } from 'crypto';

import { getRandomInt } from '../src';

describe(getRandomInt.name, () => {
it('raises same error as randomInt from crypto when max <= min', () => {
const EXPECTED_ERROR = new RangeError(
'The value of "max" is out of range. It must be greater than the value of "min" (0). Received -1',
);

expect(() => randomInt(0, -1)).toThrowError(EXPECTED_ERROR);
expect(() => getRandomInt(0, -1)).toThrowError(EXPECTED_ERROR);
});

it('returns same value when range is trivial', () => {
expect(randomInt(0, 1)).toEqual(0);
expect(getRandomInt(0, 1)).toEqual(0);

expect(randomInt(100, 101)).toEqual(100);
expect(getRandomInt(100, 101)).toEqual(100);
});

it('returns same value when range is trivial', () => {
for (let i = 0; i < 10_000; i++) {
const result = getRandomInt(0, 100);
expect(result).toBeGreaterThanOrEqual(0);
expect(result).toBeLessThan(100);
}
});
});
Loading

0 comments on commit 42529a9

Please sign in to comment.