uuidjs / uuid

Generate RFC-compliant UUIDs in JavaScript
MIT License
14.61k stars 903 forks source link

Version >=7.0.0 is not compatible with React Native / Expo #375

Closed dylancom closed 4 years ago

dylancom commented 4 years ago

When I'm upgrading from 3.4.0 to 7.0.0 an error saying it can't require "crypto" pops up in a React Native app. It used to work fine before.

VandalPaulius commented 4 years ago

+1 v7 isn't compatible with AWS Lambda as well. I get uuid: This browser does not seem to support crypto.getRandomValues(). error, which sounds exactly like @dylancompanjen error

jvpelt commented 4 years ago

Same goes for Azure functions. Getting the message:

uuid: This browser does not seem to support crypto.getRandomValues(). If you need to support this browser, please provide a custom random number generator through options.

ibc commented 4 years ago

Is this just affecting React Native? or also desktop browsers? I have like MANY libs for Node and browser that depends on uuid...

ctavan commented 4 years ago

Thanks for the reports, weโ€˜ll have to investigate.

Could anyone of you provide a minimal reproduction case? That would be extremely helpful!

ibc commented 4 years ago

I just confirm that the issue does NOT happen in browsers (using uuid 7.0.0 with browserify). So it may be something just related to react-native, but I cannot check that.

TrySound commented 4 years ago

Check this https://github.com/ai/nanoid#react-native

ctavan commented 4 years ago

@TrySound thanks for the hint, maybe we can go a similar route. Will check it out in detail.

ZebraFlesh commented 4 years ago

Could anyone of you provide a minimal reproduction case? That would be extremely helpful!

I think the issue is that node LTS (12.16.1 as of this message) does not offer the crypto.getRandomValues API. Hence anything that uses node (react-native, AWS Lambda, Azure Functions, etc.) will fail. Here's the minimally reproducible test case in a node REPL:

;{
const crypto = require('crypto');
console.log('TLS 1.3: ', crypto.constants.TLS1_3_VERSION); // prove the import works
crypto.getRandomValues();
}

Output:

TLS 1.3: 772
Uncaught TypeError: crypto.getRandomValues is not a function
TrySound commented 4 years ago

@ZebraFlesh You are right getRandomValues is not part of node crypto. Also it's not used in node version. https://github.com/uuidjs/uuid/blob/master/src/rng.js#L4 getRandomValues is used in browser version and it's correct. https://github.com/uuidjs/uuid/blob/master/src/rng-browser.js

The problem are environments. They provides own sandboxes.

ZebraFlesh commented 4 years ago

Here's what I know: performing the following code in a serverless handler which is bundled with webpack fails with the crypto.getRandomValues is not a function message:

import { v1 as uuidv1 } from 'uuid';
export const serve = async (event) => {
  const value = uuidv1();
};

Looking at @TrySound's links it's very strange to me that this would go into the browser code instead of the node variant, but it is. (And my webpack config explicitly has a target: 'node' line.)

TrySound commented 4 years ago

Very good catch!

@ctavan main does not always mean node environment and module does not always mean browser environment. There is special browser field for this. You should provide commonjs and esm for both node and browser environment.

Look how this is solved here in react-map-gl project https://github.com/uber/react-map-gl/blob/master/package.json#L17-L25

ZebraFlesh commented 4 years ago

main does not always mean node environment and module does not always mean browser environment. There is special browser field for this.

Jeez. It took a bit of digging to unpack all that, but you totally nailed it and made me smarter -- so thank you! Here's a link to npm's docs on browser; I couldn't find a concise explainer on module though (looks like it's mostly a proposal that most things are respecting, but not yet formalized in a spec).

TrySound commented 4 years ago

Here's spec https://github.com/defunctzombie/package-browser-field-spec

Also you can do the same to provide react-native specific solution https://github.com/ai/nanoid/blob/master/package.json#L30-L32

TrySound commented 4 years ago

"module" field is just a convention. Modern bundlers prefer it over "main". Though node will provide soon conditional exports as official solution. https://nodejs.org/api/esm.html#esm_conditional_exports

ctavan commented 4 years ago

I believe we have two distinct issues in this one:

  1. This module currently doesn't seem to be compatible with react-native since react-native seems to use the browser code of this library but doesn't expose the WebCrypto global crytpo object. We'll likely be able to provide a fix for this case similar as nanoid does by providing an override in the package.json's react-native field: https://github.com/ai/nanoid/blob/master/package.json#L30-L32

  2. This module apparently doesn't work in Azure functions for similar reasons: Apparently Azure functions try to invoke browser code while it should run the node code. I have created a separate issue to track the Azure functions problem: #378

With regards to react-native I'm happy to take a deeper look at the solution that nanoid chose but I'm equally happy to accept pull-requests that add such a feature.

The good news is that all of the implementations that are now broken very likely were using flawed random number generators with a much higher likelihood of UUID collisions (see https://github.com/tc39/proposal-uuid#how-unique-are-v4-uuids for examples) so we're finally discovering those implementations and now have a chance to fix them.

evdama commented 4 years ago

Same goes for Azure functions. Getting the message:

uuid: This browser does not seem to support crypto.getRandomValues(). If you need to support this browser, please provide a custom random number generator through options.

Same as for AWS, Azure, is also true for google cloud functions such as firebase cloud functions:

error from setNonceMiddleware: Error: uuid: This browser does not seem to support crypto.getRandomValues(). If you need to support this browser, please provide a custom random number generator through options.rng.

TrySound commented 4 years ago

@ctavan 3. Also main/module is confused with node/browser

ctavan commented 4 years ago

Looking at nanoid it looks like a solution for react-native is unfortunately not 100% straightforward: The random number generators used in node (crypto.randomBytes()) and the browser (crypto.getRandomValues()) are both synchronous. Hence all methods that this library exposes so far are synchronous as well.

The approach that nanoid takes is to offer async variants of the methods and then makes use of expo-random to generate cryptographically secure random numbers.

Nanoid also offers non-secure variants which use Math.random() under the hood. We have explicitly removed insecure random number generation in https://github.com/uuidjs/uuid/pull/354 with the latest major version upgrade and we will not add them for react-native.

That said there seem to be plenty of attempts on npm to provide Polyfills for either node's crypto.randomBytes() or WebCrypto crypto.getRandomValues() for react-native: https://www.npmjs.com/search?q=react%20native%20random

However I found it really hard to judge if any of these implementations satisfy the two criteria

  1. synchronous
  2. cryptographically secure pseudo random numbers

@LinusU I saw that you authored one of these libraries (https://github.com/LinusU/react-native-get-random-values) and since you have been contributing to this library as well, maybe you can shed a bit more light on the situation w.r.t. sync CSPRNGs in react-native (or anyone else who is familiar with the matter)?

slavikdenis commented 4 years ago

I think it's fixed in version 7.0.1

EugeneDraitsev commented 4 years ago

I still have the same error in 7.0.1

import { v4 as uuid4 } from 'uuid'
uuid4()
Error: uuid: This browser does not seem to support crypto.getRandomValues().
If you need to support this browser, please provide a custom random number generator through options.rng.
lorenc-tomasz commented 4 years ago

The same error in 7.0.1

ctavan commented 4 years ago

The react-native problem has not yet been fixed and as explained above a fix is not straightforward.

7.0.1 fixed problem with Node.js cloud environments.

LinusU commented 4 years ago

@ctavan I think it's very unfortunate that React Native doesn't provide a proper crypto.getRandomValues out of the box ๐Ÿ˜ž

Personally, I think that the best approach is to have instructions to install the react-native-get-random-values polyfill and load that in at the setup for React Native users. That way they get a proper RNG, and the usage would be transparent since you only have to add the global polypill once.

Something like:

React Native: If you are using this library together in React Native, you need to add a polyfill for crypto.getRandomValues. We recommend using react-native-get-random-values.

I'd be happy to update documentation in that package as well if there is anything that is unclear ๐Ÿ‘


oh, and for anyone else in this thread, if you want a fix to this problem you should be able to install react-native-get-random-values and add import 'react-native-get-random-values' to the top of your index.js. Than the uuid package should work magically!

ctavan commented 4 years ago

Thank you so much for your contributions!

I think we could go even one step further and make this work out of the box using a react-native field in package.json. However documenting how to install a Polyfill might also be a viable solution, in particular since we would define the polyfill as a peerDependency only anyways, so there would be an extra npm install involved anywaysโ€ฆ

Just one thing I'm really curious about: Does your polyfill use a random number generator on both platforms (iOS and Android) that qualifies as "cryptographically secure"? And if so, why on earth do all other polyfill libraries for react-native that I found out there only provide async methods and no sync methods (most that I found seem to be using https://www.npmjs.com/package/sjcl as a fallback for their sync implementations)?

So I'm happy to merge your changes I just want to ensure that we are getting the same quality of randomness that we get with node and webcrypto.

LinusU commented 4 years ago

@ctavan

I think we could go even one step further and make this work out of the box using a react-native field in package.json.

I'm not exactly sure what we would put in that file? Do you mean that it would require the polyfill there? ๐Ÿค”

However documenting how to install a Polyfill might also be a viable solution, in particular since we would define the polyfill as a peerDependency only anyways, so there would be an extra npm install involved anywaysโ€ฆ

I'm not sure that declaring react-native-get-random-values as a peerDependency is the best way forward, since that will give an "unmet peer dependency" warning for anyone using uuid on Node.js/browsers.

Just one thing I'm really curious about: Does your polyfill use a random number generator on both platforms (iOS and Android) that qualifies as "cryptographically secure"?

It does. It uses SecRandomCopyBytes on iOS and SecureRandom on Android.

And if so, why on earth do all other polyfill libraries for react-native that I found out there only provide async methods and no sync methods (most that I found seem to be using https://www.npmjs.com/package/sjcl as a fallback for their sync implementations)?

The fact that you can have synchronous methods in a React Native module isn't all that well documented. In fact, I only found out when I tried to add crypto.getRandomValues directly to React Native.

So I'm happy to merge your changes I just want to ensure that we are getting the same quality of randomness that we get with node and webcrypto.

Sounds good ๐Ÿ‘

LinusU commented 4 years ago

This is still a problem for people using Expo, since they cannot install native modules. Please follow progress (and add your ๐Ÿ‘ if it's affecting you) here: https://github.com/expo/expo/issues/7209

actuallymentor commented 4 years ago

Unless I am misunderstanding the docs, this is a workaround for expo:

import { v4 as uuidv4 } from 'uuid'
import * as Random from 'expo-random'

const getuuid = async f => uuidv4( { random: await Random.getRandomBytesAsync( 16 ) } )

Which would give uuidv4 a random number based on the Expo wrapper for native crypto.

ctavan commented 4 years ago

@actuallymentor I believe that this code technically works however there's a crucial difference w.r.t. to how uuid@3.x was working (and uuid@7.x is still working in all supported environments): Your example getuuid method is now async with potentially totally different performance characteristics than the sync (and usually pretty consistently fast) uuid standard implementation.

That said, if you are generating UUIDs only at a very low rate, it will probably not make a huge difference (but I would be curious about comparisons between uuid@3.x and the above async code).

actuallymentor commented 4 years ago

Your example getuuid method is now async

That was not a decision on my part but a result of the Random API being async. I would prefer to have it sync, but I need to get my expo/react-native project's uuids somehow :)

ctavan commented 4 years ago

I would prefer to have it sync, but I need to get my expo/react-native project's uuids somehow :)

Yep, I guess the best thing you can do is to express your interest in https://github.com/expo/expo/issues/7209 (already saw you ๐Ÿ‘'ed) and follow the issue over there.

mikehardy commented 4 years ago

Hi there! I use this package and just finished upgrading to v7 for a react-native/reactxp app. After I added the react-native-get-random-values polyfill in my bootstrap everything is working, so that's nice.

I just wanted to mention as the maintainer of react-native-device-info that if you recommend a native module in react-native that uses the barely-documented trick of doing blocking-synchronous calls, it breaks the common development scenario of people using the Chrome Dev Tools to do debugging (since the JS bundle runs in the browser instead of on device, and that can't be blocking/synchronous). This caused me much unexpected developer + maintainer pain.

I personally don't use the Dev tools so I personally don't care, but you may see issues related to the react-native debugger not working, and if you do, that's the root cause. Cheers :-)

LinusU commented 4 years ago

it breaks the common development scenario of people using the Chrome Dev Tools to do debugging (since the JS bundle runs in the browser instead of on device, and that can't be blocking/synchronous).

Did you try that? Because as far as I know it already works out of the box with Chrome Dev Tools since that already has crypto.getRandomValues. The polyfill shouldn't be needed then...

mikehardy commented 4 years ago

Oh my! You are absolutely right, it's in the browser so it's fine - didn't think that through - as stated I don't use them myself, just noted the use of the barely documented sync native stuff and it reminded me of the troubles I had when I used it. I think you're all set then. Thanks again for the library

LinusU commented 4 years ago

No worries! ...and I would also like to say that I would love it if we didn't need the polyfill. I've tried upstreaming this into React Native before, but they didn't see the need for it ๐Ÿ˜ž

mikehardy commented 4 years ago

Yeah I saw that :-), I think you proposed it right as the lean core initiative was taking place and my read on it was that they wanted to make sure it was possible (as a pop quiz on their infrastructure capabilities) but that it wasn't a permanent no. I chimed in for whatever it's worth

eugenemlee commented 4 years ago

I thought I should mention some other related issues i've encountered while trying to upgrade to v7 on RN. Using react-native-get-random-values breaks shortid which is used to generate route keys in React Navigation, hence breaking navigation for your app as well. This seems to only be the case on Android.

LinusU commented 4 years ago

@eugenemlee

Using react-native-get-random-values breaks shortid [...]

Hmm, this should be fixed, is there an open issue somewhere? or an explanation of the problem?

eugenemlee commented 4 years ago

@LinusU Ok, I noticed that on Android for all values of n = 3n - 1:

self.getRandomValues(new Uint8Array(n)))

Would always return a n-array where the last item was 0. So I changed the following line in RNGetRandomValuesModule.java from:

byte[] data = new byte[byteLength];

to

byte[] data = new byte[byteLength + 1];

and it works okay now. I have no idea why that would be the case, maybe you can shed some light.

LinusU commented 4 years ago

@eugenemlee crap, that is very bad, I'll push a fix now, thanks for giving me the details ๐Ÿ‘

LinusU commented 4 years ago

@eugenemlee thanks again for bringing that to my attention, I published a fix as 1.3.1 and have deprecated all other versions with an appropriate message.

To anyone else in this thread, I should say that the bug did not affect the UUID package, as it did not trigger the bug in the package. That being said, you should still update to the latest version asap so that no other part of your app is affected.

RWOverdijk commented 4 years ago

@actuallymentor thanks for your workaround, works perfect for the time being. ๐Ÿ‘

danstepanov commented 4 years ago
const getuuid = async f => uuidv4( { random: await Random.getRandomBytesAsync( 16 ) } )

What would this look like outside of Expo?

esmailbenmoussa commented 4 years ago

I installed react-native-get-random-values (1.3.1) to my react-native (0.62.1) project and I'm still getting this error msg:

Failed to run .then() [Error: crypto.getRandomValues() not supported. See https://github.com/uuidjs/uuid#getrandomvalues-not-supported]

What's the status on this issue?

LinusU commented 4 years ago

@ebm93 did you add an import statement to import react-native-get-random-values? Did you run pod install in the ios/ directory?

esmailbenmoussa commented 4 years ago

Yes and Yes,

import 'react-native-get-random-values'; import {v4 as uuidv4} from 'uuid';

console.log("random number", uuidv4())

throws:

Error: crypto.getRandomValues() not supported. See https://github.com/uuidjs/uuid#getrandomvalues-not-supported]

LinusU commented 4 years ago

@ebm93 Are you running in the debugger? In that case see #416

esmailbenmoussa commented 4 years ago

@LinusU Yes, but it still happens in release builds on my iPhone X device. However, only time uuidv4 works fine is when I'm running it in debug mode with React Native Debugger.

Very strange..

alireza-mpr commented 4 years ago

@LinusU same problem here

Update: After nuking node_modules and reinstalling with npm ci, it's working now.

@LinusU Yes, but it still happens in release builds on my iPhone X device. However, only time uuidv4 works fine is when I'm running it in debug mode with React Native Debugger.

Very strange..

PaulHaze commented 4 years ago

For what it is worth, I have got it working in Expo with UUID v7.x.x by using v5 and implementing a NAMESPACE constant and creating an id using the value of the thing to be encoding alongside your namespace constant. Basically following this:

https://github.com/uuidjs/uuid#version-5-namespace

My code looks like this: import { v5 as uuid } from 'uuid'; import NAMESPACE from '../../data/constants/uuid-namespace';

const displayFinalTerms = Object.entries( copyOnlyValues(final), ).map(([key, value]) => ( <SummaryList key={uuid(value, NAMESPACE)} title={key} summaryTerms={value} navToTerm="Structure" /> ));

It's now working fine in my project with no errors....

ctavan commented 4 years ago

@PaulHaze this is totally expected since v3/v5 UUIDs are entirely deterministic, i.e. constant input produces constant output and there are no random numbers involved. Hence: No problem for react-native/expo where the missing piece vor v1/v4 UUIDs is a cryptographically secure random number generator.

As a side note: @PaulHaze please make sure that the deterministic nature of v5 UUIDs is really what you need in your use case. If you are just using it to generate a react list key element, then you could equally just run whatever hash function over your value variable and would not really need the added complexity of v5 UUIDs (i.e. the namespace).