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

Use Math.random when debugger is attached #8

Merged
merged 4 commits into from
Apr 20, 2020

Conversation

TheAlmightyBob
Copy link
Contributor

Normally this calls a synchronous native method, which is more secure and of course the point of this library, but calling synchronous native methods is not supported when the debugger is attached.

AFAIK this only affects VSCode. Debugging with Chrome or React Native Debugger bypasses this library altogether, in favor of their global.crypto.getRandomValues. VSCode does not provide global.crypto.

The Math.random logic here is directly copied from previous versions of https://github.com/uuidjs/uuid, before they removed it (and encouraged the use of this library).

TheAlmightyBob and others added 3 commits April 19, 2020 18:33
Normally this calls a synchronous native method, which is more secure and of course the point of this library, but calling synchronous native methods is not supported when the debugger is attached.
@LinusU
Copy link
Owner

LinusU commented Apr 20, 2020

@TheAlmightyBob thank you for this!

I added two commits to 1) warn when the insecure RNG is used, and 2) make sure that it's only used when running in the debugger, __DEV__ is true in all non-release builds so thus it would fall back even when running without a debugger attached.

Would you mind reviewing my two commits? ❤️

@TheAlmightyBob
Copy link
Contributor Author

@LinusU Nice! Those changes both look good to me.
(I might be inclined to filter out the warning in my own app, but it makes sense to inform others using this)

@TheAlmightyBob
Copy link
Contributor Author

And especially thanks for reminding me that __DEV__ is true in non-release builds... I knew that but got mixed up when doing this :-P

@LinusU
Copy link
Owner

LinusU commented Apr 20, 2020

I might be inclined to filter out the warning in my own app, but it makes sense to inform others using this

Yeah, we can potentially ease up on it in the future, I just want to be very sure that we aren't using an insecure RNG somewhere we shouldn't...

If this is also just happening in VS Code as you indicated, we could potentially fix it upstream there so that this isn't needed at all :)

In fact, the best fix to get rid of the warning is probably to implement crypto.getRandomValues in the VS Code debugger!

@LinusU LinusU merged commit 053b5ac into LinusU:master Apr 20, 2020
@LinusU
Copy link
Owner

LinusU commented Apr 20, 2020

Thanks!

Released as 🚢 1.4.0 / 2020-04-20

@TheAlmightyBob
Copy link
Contributor Author

I sort of agree, but:

  1. I'm not sure if there's a particular reason that it's not available in VSCode, or if that would be an issue w/ VSCode itself or with VSCode's React Native Tools extension
  2. In a way, it's kind of weird/unexpected that Chrome is bypassing this library entirely, whereas normally running the app would not... so it could possibly be seen as good that VSCode is not providing a global context that wouldn't be available when running in release mode... it just happens to be a problem here because this implementation relies on a synchronous native module call...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants