Skip to content

Reland "Use crypo RNG for auto ID generation to reduce conflicts (#2764)" #3012

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
May 7, 2020
Merged

Reland "Use crypo RNG for auto ID generation to reduce conflicts (#2764)" #3012

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
867417c
Reland "Use crypo RNG for auto ID generation to reduce conflicts (#28…
wu-hui May 4, 2020
a2b89c2
Reland "Use crypo RNG for auto ID generation to reduce conflicts (#28…
wu-hui May 4, 2020
02592ec
Fix merge error
wu-hui May 4, 2020
17b6bc5
Address comments
wu-hui May 5, 2020
ea4967a
Suppress lint
wu-hui May 5, 2020
434e650
Address comments
wu-hui May 7, 2020
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
6 changes: 6 additions & 0 deletions packages/firestore/src/platform/platform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ export interface Platform {
/** Converts a binary string to a Base64 encoded string. */
btoa(raw: string): string;

/**
* Generates `nBytes` of random bytes. If `nBytes` is negative, an empty array
* will be returned.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the rationale for accepting negative numbers? This seems like it may be hiding bugs in the calling code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to validate input, and reject anything <=0.

*/
randomBytes(nBytes: number): Uint8Array;

/** The Platform's 'window' implementation or null if not available. */
readonly window: Window | null;

Expand Down
24 changes: 24 additions & 0 deletions packages/firestore/src/platform_browser/browser_platform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { Platform } from '../platform/platform';
import { Connection } from '../remote/connection';
import { JsonProtoSerializer } from '../remote/serializer';
import { ConnectivityMonitor } from './../remote/connectivity_monitor';

import { NoopConnectivityMonitor } from '../remote/connectivity_monitor_noop';
import { BrowserConnectivityMonitor } from './browser_connectivity_monitor';
import { WebChannelConnection } from './webchannel_connection';
Expand Down Expand Up @@ -72,4 +73,27 @@ export class BrowserPlatform implements Platform {
btoa(raw: string): string {
return btoa(raw);
}

randomBytes(nBytes: number): Uint8Array {
if (nBytes <= 0) {
return new Uint8Array();
}

// Polyfill for IE and WebWorker
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you improve this comment to explain how it affects the reader? Is the polyfill being done by the code block that follows? Or is it instructing the reader to perform some polyfilling? Consider re-writing this comment as a sentence that would be either clearly informative or clearly actionable by the reader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// eslint-disable-next-line no-restricted-globals
const crypto =
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian May 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We might also consider adding support for this shim on ReactNative: https://stackoverflow.com/questions/45301900/howto-patch-shim-crypto-getrandomvalues-for-react-native

While the fallback will make sure these environments don't break, we can possibly extend our support for crypto.

Edit: I missed that you added self && self.crypto. I blame it on the inline Linter warning which you can suppress by moving your no-any line two down :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we don't need it, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am ok with not adding support for ReactNative until the ReactNative Firebase wrapper bundles "crypto", but note that without getting "crypto" from globals, we will always fall back to Math.random() for RN.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I am filing a ticket to get crypto added for react native firestore.

Copy link

Choose a reason for hiding this comment

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

react-native-firebase participant here, just cross-linking invertase/react-native-firebase#3618

Copy link
Member

Choose a reason for hiding this comment

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

Just thought I'd quickly summerize; React Native Firebase doesn't use the JS SDK, we're a thin JS wrapper around the native Android & iOS SDKs so this change has no direct affect on us.

However our JS wrapper does use an identical random id generator - so I think we may need to port this change across in some form - we'll pick this up separately, thanks!

(this.window &&
(this.window.crypto || (this.window as any)['msCrypto'])) ||
(self && self.crypto);
Copy link
Contributor

Choose a reason for hiding this comment

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

According to https://developer.mozilla.org/en-US/docs/Web/API/Window/self, you can just write:

const crypto = typeof self !=== 'undefined' && (self.crypto || (self as any)['msCrypto']);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

const v = new Uint8Array(nBytes);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Consider a more meaningful name than v (e.g. generatedBytes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

if (!!crypto) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this double exclamation mark intentional? If not, you may want to add a unit test that would fail if Math.random() is used unintentionally.

Copy link
Contributor

Choose a reason for hiding this comment

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

This can probably just be if(crypto) since an if statement just needs a "truthy" value and not a boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

crypto.getRandomValues(v);
} else {
// Falls back to Math.random
for (let i = 0; i < nBytes; i++) {
v[i] = Math.floor(Math.random() * 256);
}
}
return v;
}
}
9 changes: 9 additions & 0 deletions packages/firestore/src/platform_node/node_platform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* limitations under the License.
*/

import { randomBytes } from 'crypto';
import { inspect } from 'util';

import { DatabaseId, DatabaseInfo } from '../core/database_info';
Expand Down Expand Up @@ -75,4 +76,12 @@ export class NodePlatform implements Platform {
btoa(raw: string): string {
return new Buffer(raw, 'binary').toString('base64');
}

randomBytes(nBytes: number): Uint8Array {
if (nBytes <= 0) {
return new Uint8Array();
}

return randomBytes(nBytes);
}
}
14 changes: 12 additions & 2 deletions packages/firestore/src/util/misc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

import { debugAssert } from './assert';
import { PlatformSupport } from '../platform/platform';

export type EventHandler<E> = (value: E) => void;
export interface Indexable {
Expand All @@ -28,8 +29,17 @@ export class AutoId {
const chars =
'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';
let autoId = '';
for (let i = 0; i < 20; i++) {
autoId += chars.charAt(Math.floor(Math.random() * chars.length));
while (autoId.length < 20) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider re-writing this loop as while (true) and then breaking out of the loop with a break statement immediately after appending the 20th character to autoId. This would avoid hardcoding 20 in two different places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rewrote the section to reduce hardcoding numbers.

const bytes = PlatformSupport.getPlatform().randomBytes(40);
for (const b of Array.from(bytes)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: You can save one copy here if you do a for(let i = 0; i < bytes.length; ++i) loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// Length of `chars` is 62. We only take bytes between 0 and 62*4-1
// (both inclusive). The value is then evenly mapped to indices of `char`
// via a modulo operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this logic is used to ensure an even distribution between all of the characters in chars. Please update the comment to explain why this logic exists because it is non-obvious.

const maxValue = 62 * 4 - 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider moving the calculation of maxValue to immediately follow the declaration of chars. IMO, having it re-calculated on each loop iteration detracts from readability because it incorrectly implies that its value may change between iterations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hardcoding 62 in three different places (including in the comment) is a bit unnerving. It would be preferable to use chars.length, for robustness. How about something like this:

const chars = '...';
const maxValue = chars.length * 4 - 1;
debugAssert(maxValue < 256);

Then the hardcoded value of 62 in the modulo operation below can be safely replaced with chars.length.

if (autoId.length < 20 && b <= maxValue) {
autoId += chars.charAt(b % 62);
}
}
}
debugAssert(autoId.length === 20, 'Invalid auto ID: ' + autoId);
return autoId;
Expand Down
4 changes: 4 additions & 0 deletions packages/firestore/test/util/test_platform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,10 @@ export class TestPlatform implements Platform {
btoa(raw: string): string {
return this.basePlatform.btoa(raw);
}

randomBytes(nBytes: number): Uint8Array {
return this.basePlatform.randomBytes(nBytes);
}
}

/** Returns true if we are running under Node. */
Expand Down