Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better random #1081

Merged
merged 2 commits into from
Aug 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions spot-client/src/common/remote-control/P2PSignalingClient.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { logger } from 'common/logger';

import { generate8Characters } from '../utils';

import P2PSignalingBase from './P2PSignalingBase';


Expand Down Expand Up @@ -134,7 +132,7 @@ export default class P2PSignalingClient extends P2PSignalingBase {
throw new Error('No PeerConnection');
}

const requestId = `${generate8Characters()}-${generate8Characters}`;
const requestId = window.crypto.randomUUID();

// FIXME is there any better way to do this?
// The 'reject' and 'resolve' methods are set when the promise is created.
Expand Down
40 changes: 2 additions & 38 deletions spot-client/src/common/utils/cryptography.js
Original file line number Diff line number Diff line change
@@ -1,45 +1,9 @@
/**
* Generates a random 8 character long string.
*
* @returns {string}
*/
export function generate8Characters() {
const buf = new Uint16Array(2);

window.crypto.getRandomValues(buf);

return `${s4(buf[0])}${s4(buf[1])}`;
}

/**
* Generate a likely-to-be-unique guid.
* Generate a GUID.
*
* @private
* @returns {string} The generated string.
*/
export function generateGuid() {
const buf = new Uint16Array(8);

window.crypto.getRandomValues(buf);

return `${s4(buf[0])}${s4(buf[1])}-${s4(buf[2])}-${s4(buf[3])}-${
s4(buf[4])}-${s4(buf[5])}${s4(buf[6])}${s4(buf[7])}`;
}

/**
* Converts the passed in number to a string and ensure it is at least 4
* characters in length, prepending 0's as needed.
*
* @param {number} num - The number to pad and convert to a string.
* @private
* @returns {string} - The number converted to a string.
*/
function s4(num) {
let ret = num.toString(16);

while (ret.length < 4) {
ret = `0${ret}`;
}

return ret;
return window.crypto.randomUUID();
}
83 changes: 79 additions & 4 deletions spot-client/src/common/utils/random.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,87 @@
/**
* Helper class to generate random values one by one, using an auxiliary
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat trick!

* pool to avoid calling getRandomValues every time.
*/
class RandomPool {
/**
* Default size for the pool.
*/
SIZE = 256;

/**
* Build a new pool.
*/
constructor() {
this._rArray = new Uint8Array(this.SIZE);
this._idx = this.SIZE; // Fill the pool on first use.
}

/**
* Gets a random number in the Uint8 range.
*
* @returns {number}
*/
getValue() {
if (this._idx > this.SIZE - 1) {
// Fill the pool.
window.crypto.getRandomValues(this._rArray);
this._idx = 0;
}

return this._rArray[this._idx++];
}
}

const pool = new RandomPool();

/**
* Choose `num` elements from `seq`, randomly.
*
* @param {string} seq - Sequence of characters, the alphabet.
* @param {number} num - The amount of characters from the alphabet we want.
* @returns {string}
*/
function randomChoice(seq, num) {
const x = seq.length - 1;
const r = new Array(num);
let len = num;

while (len--) {
// Make sure the random value is in our alphabet's range.
// eslint-disable-next-line no-bitwise
const idx = pool.getValue() & x;

r.push(seq[idx]);
}

return r.join('');
}

/**
* This alphabet removes all potential ambiguous symbols, so it's well suited for a code.
*/
const BASE32 = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ234567';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is cuel. Something comes to mind though, users won't know that they can only enter non ambiguous chars, so on the remote side when they enter the code they still might confuse 0 with O, maybe remove O and I as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to stay with the base32 standard. A monospaced font should take care of that...


/**
* A method to generate a random string, intended to be used to create a random join code.
*
* @param {number} length - The desired length of the random string.
* @returns {string}
*/
export function generateRandomString(length) {
// XXX the method may not always give desired length above 9
return Math.random()
.toString(36)
.slice(2, 2 + length);
return randomChoice(BASE32, length);
}

/**
* This alphabet is similar to BASE32 above, but includes lowercase characters too.
*/
const BASE58 = '123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz';

/**
* Generates a random 8 character long string.
*
* @returns {string}
*/
export function generate8Characters() {
return randomChoice(BASE58, 8);
}
Loading